feat(Popover): applied tokens by thatblindgeye · Pull Request #6065 · patternfly/patternfly

thatblindgeye · 2023-11-16T14:04:40Z

Work towards #5732

In addition to the below issues, will leave some comments for clarification in the files

~~The color contrast of the Warning and Custom variants colors fails in light mode; the text color is too light against the white background.~~ EDIT: use default/regular text color
The no padding variant ends up with the text visually outside of the container, due to the border radius (maybe just an example update? or a non-issue since we mention that it's intended for content that has its own padding and should be up to the consumer). EDIT: This will be up to the consumer (could update the example)

thatblindgeye · 2023-11-16T14:05:49Z

src/patternfly/components/Popover/popover.scss

+  --#{$popover}__arrow--Width: #{pf-size-prem(25px)};
+  --#{$popover}__arrow--Height: #{pf-size-prem(25px)};


There's no arrow tokens, so I just took the value of --#{$pf-global}--arrow--width-lg which is what Tooltip has for similar CSS vars

Per design, update to same size as Tooltip

thatblindgeye · 2023-11-16T14:07:05Z

src/patternfly/components/Popover/popover.scss

-  --#{$popover}__title-text--LineHeight: var(--#{$pf-global}--LineHeight--md);
-  --#{$popover}__title-text--FontFamily: var(--#{$pf-global}--FontFamily--heading);
-  --#{$popover}__title-text--FontSize: var(--#{$pf-global}--FontSize--md);
+  --#{$popover}__title-text--LineHeight: var(--pf-t--global--font--line-height--heading);


Could use another pair of eyes, but it looks like the alignment of the icon and title text is a little off with this line height, whereas if I used the ...--body line height it looks more in line. Which line height should be used here?

Per design, update the title text to body--lg and remove the title text line height

patternfly-build · 2023-11-16T14:08:09Z

Preview: https://patternfly-pr-6065.surge.sh

A11y report: https://patternfly-pr-6065-a11y.surge.sh

thatblindgeye · 2023-11-16T14:09:57Z

src/patternfly/components/Popover/popover.scss

+  --#{$popover}__close--Top: calc(var(--#{$popover}__content--PaddingTop) - var(--pf-t--global--spacer--sm)); // align top of button with top of text
+  --#{$popover}__close--Right: calc(var(--#{$popover}__content--PaddingRight)); // align right of content


For the Top var, there is no form element token so I updated the calc function to subtract the xs spacer * 1.5 (results in the current final number of a 10px inset-block-start value). If there's a better alternative I can update the calculation here.

For the Right var, I just removed the calculation. When I updated the calculations second value to the new spacer--md token the button looked too far to the right:

Here's what it looks like using the xs spacer subtracted from the --#{$popover}__content--PaddingRight out of curiosity:

thatblindgeye · 2023-11-17T14:25:19Z

Updated the no padding example slightly, applying custom CSS to a div passed to the popover body and giving it a bit more padding than normal just to have it standout.

mcoker

Looks like the arrow position needs to be adjusted, and another nit about the example - otherwise LGTM! 🐷👍

mcoker · 2023-11-27T19:29:17Z

src/patternfly/components/Popover/examples/Popover.css

+.ws-popover-no-padding-content {
+  padding-block-start: 24px;
+  padding-block-end: 24px;
+  padding-inline-start: 24px;
+  padding-inline-end: 24px;
+}


As discussed, we can just present the no padding example without any padding, which better shows what it does.

Suggested change

.ws-popover-no-padding-content {

padding-block-start: 24px;

padding-block-end: 24px;

padding-inline-start: 24px;

padding-inline-end: 24px;

}

.ws-popover-no-padding-content {

padding-block-start: 24px;

padding-block-end: 24px;

padding-inline-start: 24px;

padding-inline-end: 24px;

}

mcoker · 2023-11-27T19:29:48Z

src/patternfly/components/Popover/examples/Popover.md

+      <div class="ws-popover-no-padding-content">
+        This popover has no padding itself. Instead it is intended for use with content that has its own padding that prevents it from touching the popover container's edges.
+      </div>


Suggested change

<div class="ws-popover-no-padding-content">

This popover has no padding itself. Instead it is intended for use with content that has its own padding that prevents it from touching the popover container's edges.

</div>

This popover has no padding itself. Instead it is intended for use with content that has its own padding that prevents it from touching the popover container's edges.

thatblindgeye · 2023-11-28T14:01:10Z

src/patternfly/components/Popover/popover.scss

+  --#{$popover}__arrow--m-inline-top--Top: var(--pf-t--global--border--radius--medium);
+  --#{$popover}__arrow--m-inline-bottom--Bottom: var(--pf-t--global--border--radius--medium);
+  --#{$popover}__arrow--m-block-left--Left: var(--pf-t--global--border--radius--medium);
+  --#{$popover}__arrow--m-block-right--Right: var(--pf-t--global--border--radius--medium);


Not 100% sure how I feel about the naming so open to other ideas. Could also just apply the border radius token directly where the top/right/bottom/left vars are reassigned rather than adding new css vars, but since we may want to do something similar in Tooltip it would make sense to do the same here.

srambach

Just 1.5 comments 😆

srambach · 2023-11-28T18:33:26Z

src/patternfly/components/Popover/popover.scss

-  --#{$popover}__title-text--FontFamily: var(--#{$pf-global}--FontFamily--heading);
-  --#{$popover}__title-text--FontSize: var(--#{$pf-global}--FontSize--md);
+  --#{$popover}__title-text--Color: var(--pf-t--global--text--color--regular);
+  --#{$popover}__title-text--FontFamily: var(--pf-t--global--font--family--heading);


Should this be the body font family token?

yeah the popover title should be using semibold body-large text!

srambach · 2023-11-28T18:48:38Z

src/patternfly/components/Popover/popover.scss

-  --#{$popover}__close--Top: calc(var(--#{$popover}__content--PaddingTop) - var(--#{$pf-global}--spacer--form-element)); // align top of button with top of text
-  --#{$popover}__close--Right: calc(var(--#{$popover}__content--PaddingRight) - var(--#{$pf-global}--spacer--md)); // align right of content
-  --#{$popover}__close--sibling--PaddingRight: var(--#{$pf-global}--spacer--2xl);
+  --#{$popover}__close--Top: calc(var(--#{$popover}__content--PaddingTop) - (var(--pf-t--global--spacer--xs) * 1.5)); // align top of button with top of text


I'm not wild about this magic number but it does line it up nicely. I'm not sure if future icon box changes are going to affect this. I've been putting a // TODO at all the --#{$pf-global}--spacer--form-element uses so we can revisit.

srambach

⭐

patternfly-build · 2023-11-29T18:59:55Z

🎉 This PR is included in version 6.0.0-alpha.24 🎉

The release is available on:

Your semantic-release bot 📦🚀

* feat(breaker): bump this version to 6.0 (patternfly#5800) * fix: use prerelease tag format with dry run * fix: turn off dryrun * feat(tokens): add initial global tokens (patternfly#5843) * feat(Button): adds tokens and applies to buttons (patternfly#5908) * feat(table): apply tokens from hackathon (patternfly#5965) * feat(menu-toggle): add in work from hackathon (patternfly#5931) * feat(nav): adds tokens to page and nav (patternfly#5942) * chore(tokens): update tokens from figma (patternfly#5970) * feat(page): page and masthead updates from hackathon (patternfly#5968) * fix(tokens): add dark theme blend mode token (patternfly#5992) * feat(notificationBadge): refactor with tokens (patternfly#5991) * chore(masthead ): applied tokens (patternfly#6004) * feat(alert): apply tokens (patternfly#6002) * feat(alert): apply tokens * feat(alert): remove unused background color * feat(page): apply tokens (patternfly#6021) * feat(page): apply tokens * feat(page): undo menu__list change * feat(page): tweak spacing * fix(table): fix clickable overlay (patternfly#6025) * fix(box-shadow): typo in box shadow token names (patternfly#6039) * fix(badge): adds tokens, dropdown toggle icon (patternfly#6037) * feat(hint): apply tokens (patternfly#6030) * feat(hint): apply tokens * feat(hint): fix typo * feat(hint): update spacing * feat(hint): update border radius * feat(hint): fix heading font * feat(text, title, list): apply tokens (patternfly#6031) * feat(page): remove padding on nav sidebar (patternfly#6053) * fix(tokens): adjust font-weight base for headings (patternfly#6070) * feat(login): apply tokens (patternfly#6028) * feat(modal): add tokens to modal and about modal (patternfly#6042) * fix(font): use variable font, add opt-in for legacy font (patternfly#6073) * feat(ExpandableSection): added tokens (patternfly#6063) * feat(Tooltip): applied tokens (patternfly#6064) * feat(Popover): applied tokens (patternfly#6065) * feat(BackToTop): refactor with tokens (patternfly#6085) * feat(SkipToContent): refactor with tokens (patternfly#6090) * feat(Accordion): applied tokens (patternfly#6058) * feat(banner): refactor with tokens (patternfly#6081) * feat(InlineEdit): applied tokens Took a stab at applying the tokens here. There are some items that will be fixed with icon buttons. * chore(tokens): updates tokens from figma (patternfly#6103) * chore(tokens): updating usage of new font tokens (patternfly#6111) * feat(breadcrumb): apply tokens (patternfly#6060) * feat(HelperText): apply tokens (patternfly#6118) * feat(HelperText): first tokens * feat(HelperText): tokens changes * feat(HelperText): fixes * feat(Pagination): applied tokens (patternfly#6089) * feat(Pagination): applied tokens * Removed themes directory * Removed button and menutoggle styles, added wrapper * Removed padding vars for pagination container * Updated tokens and vars for padding * fix(Pagination): updated property order * Updated var names to remove md text * feat(avatar): apply tokens (patternfly#6122) * chore(ExpandableSection): updated Penta docs and focus/hover styling (patternfly#6124) * feat(EmptyState): apply tokens (patternfly#6056) * fix(form): keep form groups/controls from growing too wide (patternfly#6109) * chore(deps): update dependency @patternfly/react-core to v5.1.2 (patternfly#6113) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency @patternfly/react-code-editor to v5.1.2 (patternfly#6112) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency @patternfly/react-table to v5.1.2 (patternfly#6114) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(card/menu-toggle/check): alignment bugs (patternfly#6110) * feat(SimpleList): pull in new v6 branch (patternfly#6128) * chore: turn on dryrun until release fixed [ci skip] --------- Co-authored-by: Sarah Rambacher <srambach@redhat.com> Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com> Co-authored-by: Matt Nolting <matthew.nolting@gmail.com> Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com> Co-authored-by: Austin Sullivan <ausulliv@redhat.com> Co-authored-by: Andrew Ronaldson <aronalds@redhat.com> Co-authored-by: kmcfaul <45077788+kmcfaul@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

patternfly-build · 2024-01-14T14:40:08Z

🎉 This PR is included in version 6.0.0-alpha.61 🎉

The release is available on:

Your semantic-release bot 📦🚀

thatblindgeye commented Nov 16, 2023

View reviewed changes

thatblindgeye requested review from a team, lboehling and mcoker and removed request for a team November 16, 2023 14:11

thatblindgeye linked an issue Nov 16, 2023 that may be closed by this pull request

Consume tokens: Tooltip and Popover #5732

Closed

thatblindgeye force-pushed the popoverPenta branch from 39fd4c2 to be922d3 Compare November 17, 2023 14:24

mcoker requested a review from srambach November 21, 2023 22:02

mcoker approved these changes Nov 27, 2023

View reviewed changes

Eric Olkowski added 3 commits November 28, 2023 08:19

feat(Popover): applied tokens

f7baaea

Updated tokens, no-padding example

a320b90

Updated arrow corner placement

9c48ec5

thatblindgeye force-pushed the popoverPenta branch from be922d3 to 9c48ec5 Compare November 28, 2023 13:58

thatblindgeye commented Nov 28, 2023

View reviewed changes

srambach reviewed Nov 28, 2023

View reviewed changes

Updated font tokens for title text

fc16f71

thatblindgeye requested a review from srambach November 29, 2023 14:48

srambach approved these changes Nov 29, 2023

View reviewed changes

mcoker merged commit b70ba54 into patternfly:v6 Nov 29, 2023

patternfly-build added the released on @alpha label Nov 29, 2023

mcoker pushed a commit to mcoker/patternfly that referenced this pull request Dec 1, 2023

feat(Popover): applied tokens (patternfly#6065)

058dfa7

wise-king-sullyman mentioned this pull request Dec 4, 2023

Consume tokens: Tooltip and Popover #5732

Closed

nicolethoen pushed a commit that referenced this pull request Dec 12, 2023

feat(Popover): applied tokens (#6065)

1e1135b

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Dec 12, 2023

feat(Popover): applied tokens (patternfly#6065)

020832b

nicolethoen pushed a commit that referenced this pull request Dec 12, 2023

feat(Popover): applied tokens (#6065)

2748088

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Dec 19, 2023

feat(Popover): applied tokens (patternfly#6065)

24cf6f4

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Dec 27, 2023

feat(Popover): applied tokens (patternfly#6065)

2951284

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 2, 2024

feat(Popover): applied tokens (patternfly#6065)

3a9fdce

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 3, 2024

feat(Popover): applied tokens (patternfly#6065)

433c697

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 4, 2024

feat(Popover): applied tokens (patternfly#6065)

954700d

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 15, 2024

feat(Popover): applied tokens (patternfly#6065)

a146eb6

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 15, 2024

feat(Popover): applied tokens (patternfly#6065)

b24e69e

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 15, 2024

feat(Popover): applied tokens (patternfly#6065)

4c1bac0

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 15, 2024

feat(Popover): applied tokens (patternfly#6065)

9df7c9e

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 15, 2024

feat(Popover): applied tokens (patternfly#6065)

c108aea

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 15, 2024

feat(Popover): applied tokens (patternfly#6065)

bc647c6

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 16, 2024

feat(Popover): applied tokens (patternfly#6065)

7f56124

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 16, 2024

feat(Popover): applied tokens (patternfly#6065)

6efc175

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 16, 2024

feat(Popover): applied tokens (patternfly#6065)

4839623

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jan 16, 2024

feat(Popover): applied tokens (patternfly#6065)

b565a26

patternfly-build mentioned this pull request Apr 16, 2025

The automated release is failing 🚨 #7464

Closed

		--#{$popover}__arrow--Width: #{pf-size-prem(25px)};
		--#{$popover}__arrow--Height: #{pf-size-prem(25px)};

		--#{$popover}__close--Top: calc(var(--#{$popover}__content--PaddingTop) - var(--pf-t--global--spacer--sm)); // align top of button with top of text
		--#{$popover}__close--Right: calc(var(--#{$popover}__content--PaddingRight)); // align right of content

Conversation

thatblindgeye commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatblindgeye Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye commented Nov 17, 2023

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srambach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srambach left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Nov 29, 2023

Uh oh!

patternfly-build commented Jan 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thatblindgeye commented Nov 16, 2023 •

edited

Loading

thatblindgeye Nov 16, 2023 •

edited

Loading

thatblindgeye Nov 16, 2023 •

edited

Loading

patternfly-build commented Nov 16, 2023 •

edited

Loading

thatblindgeye Nov 16, 2023 •

edited

Loading