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: 5134bf8

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.53 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.36 KB	🔴 ⬆ 0.28 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.36 KB	🔴 ⬆ 0.28 KB (1.71%)
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

marissahuysentruyt · 2025-01-13T16:22:07Z

components/picker/stories/template.js

 		isOpen: isOpen && !isDisabled && !isLoading,
 		withTip: false,
-		position: "bottom",
+		position: "bottom-start",


bottom-start is from main & s2-foundations-redux. There's not explicit direction on the popover's positioning within picker that I found, but I don't think I see an issue with this change. Combobox, action menu, contextual help, date picker all use bottom-start.

The change here I thought then required the CSS selector changes, so if we're not cool with that right now, I'm happy to revert if there are any concerns.

marissahuysentruyt · 2025-01-13T16:30:37Z

components/coachmark/index.css

-	--mod-popover-corner-radius: var(--spectrum-corner-radius-100);
-	--mod-popover-content-area-spacing-vertical: 0;
+	--mod-popover-corner-radius: var(--spectrum-corner-radius-large-default)
+	--mod-popover-content-area-spacing: 0;


I updated this mod name because I changed popover-content-area-spacing in the popover CSS- there's now padding on all sides of popover instead of just on the top & bottom. However, I am unsure if this is actually the correct styling for the popovers in coachmark. Is this something to address in the coachmark PR #3412? Regardless, we do have this captured in CSS-1067

With the new padding in Popover (especially noticeable in the action menu popover), which means removing --mod-popover-content-area-spacing completely.

Keeping --mod-popover-content-area-spacing: 0, which results in no padding

We might need something like this:
padding: var(--mod-popover-content-area-spacing, var(--spectrum-popover-content-area-spacing)) 0; to mimic the old popover vertical padding. What do y'all think?

- 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

- 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

- 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

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 3 times, most recently from cdb180d to 27d01df Compare December 4, 2024 14:54

castastrophe force-pushed the spectrum-two branch 6 times, most recently from fc916f1 to 9d7f088 Compare December 29, 2024 19:50

marissahuysentruyt added wip This is a work in progress, don't judge. and removed ready-for-review labels Jan 2, 2025

marissahuysentruyt force-pushed the marissahuysentruyt/css-615-s2-popover branch from 3c6ebd8 to cc7713e Compare January 10, 2025 20:22

marissahuysentruyt commented Jan 13, 2025

View reviewed changes

marissahuysentruyt force-pushed the marissahuysentruyt/css-615-s2-popover branch 3 times, most recently from 7d6ce16 to 1c99e9c Compare January 13, 2025 17:11

marissahuysentruyt requested review from jawinn and rise-erpelding January 13, 2025 17:21

marissahuysentruyt added ready-for-review and removed wip This is a work in progress, don't judge. labels Jan 13, 2025

marissahuysentruyt force-pushed the marissahuysentruyt/css-615-s2-popover branch from 1c99e9c to ec1c97f Compare January 16, 2025 17:30

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

marissahuysentruyt added 3 commits January 24, 2025 11:44

feat(popover): new S2 popover styles

dd11010

- 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

fix(coachmark): nested popover style updates

418cd77

- 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

5134bf8

- 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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(popover): migrate s2 popover #3365

feat(popover): migrate s2 popover #3365

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

coachmark

picker

popover

github-actions bot commented Nov 5, 2024 •

edited

Loading

marissahuysentruyt Jan 13, 2025 •

edited

Loading

marissahuysentruyt Jan 13, 2025 •

edited

Loading

feat(popover): migrate s2 popover #3365

Are you sure you want to change the base?

feat(popover): migrate s2 popover #3365

Conversation

marissahuysentruyt commented Nov 4, 2024 • edited Loading

Description

Popover Mods

Notes

Designs

Jira

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

changeset-bot bot commented Nov 4, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Nov 4, 2024 • edited Loading

File metrics

Summary

coachmark

picker

popover

github-actions bot commented Nov 5, 2024 • edited Loading

marissahuysentruyt Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

marissahuysentruyt Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

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

marissahuysentruyt Jan 13, 2025 •

edited

Loading

marissahuysentruyt Jan 13, 2025 •

edited

Loading