feat(popover): migrate s2 popover #3365

marissahuysentruyt · 2024-11-04T18:49:30Z

Description

Welcome to the new and improved S2 popover! 🥳

This PR updates tokens used in the popover component. The work includes new S2 tokens specifically for colors, padding and the border radius for popover. There is also a transparent border in light mode and a visible border in dark mode.

There are several components that have popover as a subcomponent. Those were updated with just enough additional code to make them "work." Coachmark and picker did have some additional CSS needs, particularly relating to the new popover corner rounding, and the new bottom-start positioning.

Popover Mods

Some mods have been renamed to match their new token names, or better reflect their purposes:

Old mod name	New mod name
`--mod-popover-content-area-spacing-vertical`	`--mod-popover-content-area-spacing`
`--mod-popover-shadow-blur`	`--mod-popover-drop-shadow-blur`
`--mod-popover-shadow-color`	`--mod-popover-drop-shadow-color`
`--mod-popover-shadow-horizontal`	`--mod-popover-drop-shadow-x`
`--mod-popover-shadow-vertical`	`--mod-popover-drop-shadow-y`

Notes

the background color and the border radius of the menu items is a known bug. It will be addressed in a separate branch.
there's a few coach mark questions that are still outstanding as well, including the needed drop shadow on the action menu popover (css-1066), and the padding (as noted in a few comments).
the picker 's popover styling is a known issue. I attempted to implement something like:

    /* popover passthroughs */
    --mod-popover-animation-distance: var(--spectrum-picker-spacing-picker-to-popover);

but the cascade doesn't quite work because of the markup differences. CSS-1065 captures some of the thoughts and issues, and Josh brought up some good questions/context in his comment below in regards to nesting elements and SWC.

Designs

S2 Popover Token specs
S2 / Desktop Popover

Jira

CSS-615

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

Before:

After:

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts other components, I have tested to make sure they don't break.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

changeset-bot · 2024-11-04T18:49:34Z

🦋 Changeset detected

Latest commit: 1fac2ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@spectrum-css/picker	Patch
@spectrum-css/popover	Major
@spectrum-css/coachmark	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-11-04T18:52:28Z

File metrics

Summary

Total size: 2.23 MB^*
Total change (Δ): 🔴 ⬆ 0.54 KB (0.02%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package	Size	Δ
coachmark	5.48 KB	🔴 ⬆ < 0.01 KB
picker	28.63 KB	🔴 ⬆ 0.03 KB
popover	16.37 KB	🔴 ⬆ 0.29 KB

Details

coachmark

Filename	Head	Compared to base
index.css	5.48 KB	🔴 ⬆ < 0.01 KB (0.02%)
metadata.json	3.20 KB	🔴 ⬆ < 0.01 KB (0.03%)

picker

Filename	Head	Compared to base
index.css	28.63 KB	🔴 ⬆ 0.03 KB (0.10%)
metadata.json	14.22 KB	🔴 ⬆ 0.02 KB (0.17%)

popover

Filename	Head	Compared to base
index.css	16.37 KB	🔴 ⬆ 0.29 KB (1.76%)
metadata.json	8.10 KB	🔴 ⬆ 0.19 KB (2.33%)

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions · 2024-11-05T15:06:40Z

🚀 Deployed on https://pr-3365--spectrum-css.netlify.app

components/popover/stories/template.js

components/actionbar/stories/actionbar.stories.js

components/actionbar/stories/template.js

components/coachmark/index.css

components/popover/stories/popover.stories.js

rise-erpelding

I think this is good to go! Thanks for all your hard work on this, and for all the follow-up cards that you've made to explore some of the other complexities that came about with this migration - those cover a whole lot of the things that I noticed reviewing this.

I made a tiny callout about maybe linking Down state docs if you want, but I have a couple of other non-blocking questions/maybe concerns:

I'm honestly a little puzzled as to why the left/right-positioned popovers with tip are so far away from the source compared to the top/bottom-positioned popovers with tip. This isn't anything you introduced, because I saw it on main too! To me, this seems like it could be a bug - the spacing is set based on popover-pointer-width if it's side-positioned, and based on popover-pointer-height if it's vertically positioned from its source, but popover-pointer-width and popover-pointer-height don't ever change, if I'm not mistaken - it's just that for side-positioned popovers inline-size is set to the height and otherwise it's set to the width.
It also totally feels weird that we get popovers that look like this in rtl where the tip is way away from the source, but that would be a reason to use the logical position, I guess, so we probably don't need to take any further action there.
Lastly, I know you have a card for the drop-shadows as pertaining to coachmark already, but I can't help but notice how much bigger the shadows look compared to Figma. I know I've done my fair share of digging and I think the mechanisms used to render the shadows in Figma are completely different than what is used in CSS for filter: drop-shadow() but that would be something that I'd love for us to standardize further at some point in the future.

That's all I've got! Nice work, once again!

components/popover/stories/popover.stories.js

rise-erpelding · 2025-01-29T18:45:58Z

components/popover/index.css

@@ -144,7 +144,7 @@
 	&.spectrum-Popover--withTip {
 		/* tip size minus where it overlaps with popover border */
 		/* stylelint-disable-next-line csstools/use-logical -- intentional, right stays on the same side even for RTL */
-		margin-left: calc(var(--mod-popover-pointer-width, var(--spectrum-popover-pointer-width)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width)));
+		margin-left: calc(var(--mod-popover-animation-distance, var(--spectrum-popover-animation-distance)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width)));


This change looks good! I'm wondering if popover-animation-distance is the token we want but some of the context may have left my brain since yesterday--the top/bottom positions are still using popover-pointer-height while the side positions use popover-animation-distance, right? It all computes to the same value in the end, but I'm wondering if the tokens ever change their values if that leaves room for inconsistency down the road?

Good points! So, I've used the popover-pointer-height token in place of the popover-animatino-distance`, which makes much more sense I think.

To address your other comments from yesterday:

There are little documentation blurbs on the docs page for the positions that don't adapt to RTL. "* Text direction does not affect the position" and "* Changes side with text direction (like a logical property)". Is that offer enough clarity on the expected behaviors for users when they use particular positions?

I agree on the drop shadow. I thought maybe that our popover shadows were just looking "bigger" than the shadows in Figma because our background color is white, while Figma's background colors are light gray but...it's not light gray in all the files so that argument is nothing 😆 We have that card to document that we know it looks maybe a little funny, so I don't think I'm going to address anything further in this PR. Does that sound ok?

jawinn

Nice work working through this one. Approved!
I didn't see any issues. Just two tiny docs wording nitpicks here.

components/popover/stories/popover.stories.js

- new tokens and token values used for popover - create changeset - rebuild metadata.json - adjust popoverWidth and popoverHeight args to accommodate new padding - update test file with new popoverWidth and popoverHeight - correct side aligned popovers distance to source * fix(coachmark): nested popover style updates - update the border radius of the popover nested in coachmark to match S2 popover specs - update popover `--mod-popover-content-area-spacing` variable name (previously --mod-popover-content-area-spacing-vertical) - rebuild metadata.json * fix(picker): update popover selector class - with the new default position for popover, the selector class for the popover in picker also needed to be updated. (--bottom-start) - create changeset - rebuild metadata.json * docs(popover): fixup some docs wording - add down state link

feat(popover): new S2 popover styles - new tokens and token values used for popover - create changeset - rebuild metadata.json - adjust popoverWidth and popoverHeight args to accommodate new padding - update test file with new popoverWidth and popoverHeight - correct side aligned popovers distance to source * fix(coachmark): nested popover style updates - update the border radius of the popover nested in coachmark to match S2 popover specs - update popover `--mod-popover-content-area-spacing` variable name (previously --mod-popover-content-area-spacing-vertical) - rebuild metadata.json * fix(picker): update popover selector class - with the new default position for popover, the selector class for the popover in picker also needed to be updated. (--bottom-start) - create changeset - rebuild metadata.json * docs(popover): fixup some docs wording - add down state link

marissahuysentruyt self-assigned this Nov 5, 2024

marissahuysentruyt added wip This is a work in progress, don't judge. do not merge A flag for a branch indicating it should not be merged. skip_vrt Add to a PR to skip running VRT (but still pass the action) S2 Spectrum 2 labels Nov 5, 2024

marissahuysentruyt force-pushed the marissahuysentruyt/css-615-s2-popover branch from c982e9c to d9214ec Compare November 6, 2024 21:09

marissahuysentruyt commented Nov 6, 2024

View reviewed changes

components/popover/stories/template.js Outdated Show resolved Hide resolved

marissahuysentruyt marked this pull request as ready for review November 6, 2024 22:14

marissahuysentruyt added ready-for-review and removed wip This is a work in progress, don't judge. do not merge A flag for a branch indicating it should not be merged. labels Nov 6, 2024

marissahuysentruyt requested review from cdransf, jawinn, castastrophe, 5t3ph, rise-erpelding and aramos-adobe November 6, 2024 22:15

marissahuysentruyt marked this pull request as draft November 6, 2024 22:18

marissahuysentruyt added wip This is a work in progress, don't judge. and removed ready-for-review labels Nov 6, 2024

marissahuysentruyt commented Nov 7, 2024

View reviewed changes

components/actionbar/stories/actionbar.stories.js Outdated Show resolved Hide resolved

marissahuysentruyt commented Nov 7, 2024

View reviewed changes

components/actionbar/stories/template.js Outdated Show resolved Hide resolved

marissahuysentruyt commented Nov 7, 2024

View reviewed changes

components/coachmark/index.css Outdated Show resolved Hide resolved

marissahuysentruyt force-pushed the marissahuysentruyt/css-615-s2-popover branch 3 times, most recently from 9eacd5d to b2e341b Compare November 8, 2024 18:06

marissahuysentruyt commented Nov 8, 2024

View reviewed changes

components/popover/stories/popover.stories.js Outdated Show resolved Hide resolved

castastrophe force-pushed the spectrum-two branch 4 times, most recently from 7e783b6 to e3585cd Compare January 22, 2025 17:44

marissahuysentruyt force-pushed the marissahuysentruyt/css-615-s2-popover branch from ec1c97f to 5134bf8 Compare January 24, 2025 16:51

aramos-adobe approved these changes Jan 27, 2025

View reviewed changes

rise-erpelding approved these changes Jan 28, 2025

View reviewed changes

components/popover/stories/popover.stories.js Outdated Show resolved Hide resolved

marissahuysentruyt force-pushed the marissahuysentruyt/css-615-s2-popover branch 2 times, most recently from 3958f41 to 1fac2ba Compare January 29, 2025 17:36

rise-erpelding reviewed Jan 29, 2025

View reviewed changes

marissahuysentruyt force-pushed the marissahuysentruyt/css-615-s2-popover branch from 1fac2ba to 8df58b8 Compare January 30, 2025 15:14

jawinn approved these changes Jan 30, 2025

View reviewed changes

components/popover/stories/popover.stories.js Outdated Show resolved Hide resolved

components/popover/stories/popover.stories.js Outdated Show resolved Hide resolved

marissahuysentruyt force-pushed the marissahuysentruyt/css-615-s2-popover branch from 8df58b8 to a3aeafd Compare January 30, 2025 21:39

marissahuysentruyt merged commit 520a8a4 into spectrum-two Jan 31, 2025
12 checks passed

marissahuysentruyt deleted the marissahuysentruyt/css-615-s2-popover branch January 31, 2025 16:46

github-actions bot mentioned this pull request Feb 5, 2025

chore: release (next) #3537

Closed

castastrophe mentioned this pull request Apr 15, 2025

feat(tokens,card): update to @adobe/spectrum-tokens@13.3.0 #3669

Merged

12 tasks

This was referenced May 16, 2025

chore(swatch,menu,swatchgroup): fix reverted metadata assets #3752

Merged

chore: merge latest updates from main #3777

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(popover): migrate s2 popover #3365

feat(popover): migrate s2 popover #3365

Uh oh!

marissahuysentruyt commented Nov 4, 2024 •

edited

Loading

Uh oh!

changeset-bot bot commented Nov 4, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Nov 4, 2024 •

edited

Loading

coachmark

picker

popover

Uh oh!

github-actions bot commented Nov 5, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rise-erpelding left a comment

Uh oh!

Uh oh!

rise-erpelding Jan 29, 2025

Uh oh!

marissahuysentruyt Jan 30, 2025

Uh oh!

jawinn left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat(popover): migrate s2 popover #3365

feat(popover): migrate s2 popover #3365

Uh oh!

Conversation

marissahuysentruyt commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Popover Mods

Notes

Designs

Jira

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

Uh oh!

changeset-bot bot commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File metrics

Summary

coachmark

picker

popover

Uh oh!

github-actions bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rise-erpelding Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

marissahuysentruyt Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

jawinn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marissahuysentruyt commented Nov 4, 2024 •

edited

Loading

changeset-bot bot commented Nov 4, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024 •

edited

Loading

github-actions bot commented Nov 5, 2024 •

edited

Loading