feat(popover): migrate s2 popover #3365
Conversation
🦋 Changeset detectedLatest commit: 3ee1873 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
File metricsSummaryTotal size: 4.30 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailscoachmark
colorarea
picker
popover
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
marissahuysentruyt
added
wip
|
🚀 Deployed on https://pr-3365--spectrum-css.netlify.app |
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-615-s2-popover
branch
from
c982e9c
to
d9214ec
Compare
| switch (position) { | ||
| case "top": | ||
| popoverWrapperStyles["inline-size"] = "var(--spectrum-popover-width)"; | ||
| popoverAlignment["inset-block-end"] = "100%"; |
There was a problem hiding this comment.
I removed 2 lines from these test cases about "inset-inline-start" being set to "0". Something about those 2 lines was uncentering the top and bottom positioning of the popover. I was wondering if maybe it had to do with the additional horizontal padding popover has now?
In the browser on production, toggling that property on and off doesn't affect the position of the popover, but in this branch it did. Am I missing something in the CSS which would allow me to keep these 2 lines?
Screen.Recording.2024-11-06.at.4.54.36.PM.mov
marissahuysentruyt
added
ready-for-review
and removed
wip
marissahuysentruyt
requested review from
cdransf,
jawinn,
castastrophe,
5t3ph,
rise-erpelding and
aramos-adobe
marissahuysentruyt
added
wip
| @@ -68,7 +68,7 @@ export default { | |||
| parameters: { | |||
| actions: { | |||
| handles: [ | |||
| ...Popover.parameters.actions.handles, | |||
| ...(Popover?.parameters?.actions?.handles ?? []), | |||
There was a problem hiding this comment.
This is pulled over from main. I tried to pull the least amount of code changes in order to get them all to render.
| @@ -68,6 +68,8 @@ export const Template = ({ | |||
| ], | |||
| }), | |||
| ], | |||
| popoverHeight: 42, | |||
There was a problem hiding this comment.
Same here: this is pulled over from main. Most of the component template/story changes (that aren't popover) are just enough to get them to render properly(ish). I did mark the popover commits with [from main] before I started any new work for S2.
| & .spectrum-Popover { | ||
| --mod-popover-filter: drop-shadow(var(--mod-popover-drop-shadow-x, var(--spectrum-popover-drop-shadow-x)) var(--mod-popover-drop-shadow-y, var(--spectrum-popover-drop-shadow-y)) var(--mod-popover-drop-shadow-blur, var(--spectrum-popover-drop-shadow-blur)) var(--mod-popover-drop-shadow-color, var(--spectrum-popover-drop-shadow-color))); | ||
| } |
There was a problem hiding this comment.
Currently, none of these variables are defined. I just copied them verbatim from the popover CSS, BUT it's doing what I'd like to do, which is undefining the --mod-popover-filter.
I was trying to add the drop shadow back onto this popover, since it's lying on top of the rest of the coach mark's popover. I assume that's what is supposed to happen given this note in Figma: "Containers that appear on top of content, such as menus and tooltips, have drop shadows to show elevation," but then also the CSS comment to "prevent nested popovers from affecting each others drop shadow filters" (found here)
I would love to pair on this to get some help. Right now, the drop shadow on the right side of the action menu popover is "doubled up", with the drop shadow that's on the coach mark popover. I think it still needs the drop shadow when it's placed on top of other content, particularly in light mode.
Screen.Recording.2024-11-08.at.11.25.14.AM.mov
There was a problem hiding this comment.
Doing this is also introducing extra mods to coach mark and I'm not sure we need to do that.
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-615-s2-popover
branch
3 times, most recently
from
9eacd5d
to
b2e341b
Compare
| isOpen: true, | ||
| customStyles: { | ||
| "margin-inline-start": "136px", | ||
| "margin-block-start": "32px" | ||
| "margin": "var(--spectrum-popover-nested-container-spacing)", |
There was a problem hiding this comment.
This was my attempt at adding the "container padding" that is outlined in Figma. It felt weird to put those styles on the popover itself, i.e.:
& > .spectrum-Popover {
margin: var(--spectrum-popover-nested-container-spacing);
}
...since it certainly feels like they belong on the containing element. So that's what this is doing- putting that container padding on the popover wrapper.
And then users can add those styles to the wrapper if they need. Any thoughts on where these styles are supposed to go?
There was a problem hiding this comment.
Great question! I don't really have an answer, but I did look into it and will share what I've found so far:
Strangely enough, you asked about this exact thing when we did S2 for Tooltip, which has the same container padding token in its spec, and there I think we decided that it was sort of out of our hands to ensure that that token was used in the implementation.
It's also worth mentioning that, if I dig through the wiki for the S1 specs, I can find the Popover S1 token specs in the XD file from the Sharepoint, and the container padding doesn't change from S1 to S2, and it doesn't seem like it was implemented anywhere in S1 (it's the same case for tooltip, if you're curious).
I think it's fine if we want to add some margin to ensure that there's spacing here, but I have a couple of follow-up questions about that:
- Is this
--spectrum-popover-nested-container-spacingcustom prop meant to be used for all container spacing, or is there a reason it's only used for the nested container/only named for nested container spacing? - Was the intention to put this margin on the wrapper? It looks like this margin is ending up on the popover (on the same element as
.spectrum-Popover), not the wrapper, but applying them topopoverWrapperStylesinstead puts the margin on the action button, which isn't what we want either. It seems like we'd probably want some space here anyway (maybe?) but I'm not sure we need to use the token (but if I'm missing something let me know!).
- As far as I can tell, the
--spectrum-popover-nested-container-spacingis defined in index.css then not used in index.css, is that going to cause some linting issues inmain? (I would have thought there would be some in this branch too, but I didn't see any.)
Does any of that help here?
There was a problem hiding this comment.
I am totally fine with not implementing the container padding, but I suppose the question is the same for this specific, nested popover story.
Is this --spectrum-popover-nested-container-spacing custom prop meant to be used for all container spacing, or is there a reason it's only used for the nested container/only named for nested container spacing?
I don't believe this should be used for all container spacing. I'm using it only for the nested popover because those 2 popovers "touch" now. The spacing between the trigger and the popover itself is dictated by the animation-distance. That animation distance if from the trigger, which in this case is 8px from the action buttons. But now with the new, additional padding popover has (which is also 8px), the .spectrum-Popover divs "touch."
I suppose instead of spacing-100, I could use the animation-distance token?
without --spectrum-popover-nested-container-spacing:
with --spectrum-popover-nested-container-spacing:
As far as I can tell, the --spectrum-popover-nested-container-spacing is defined in index.css then not used in index.css, is that going to cause some linting issues in main? (I would have thought there would be some in this branch too, but I didn't see any.)
Correct, I didn't actually use it in the CSS. I only used it in the stories file, so I moved it and just defined it in the stories file too. Good call.
Let me know if this solution also addresses your second question about not needing to use that token! 3ee1873
There was a problem hiding this comment.
This is looking great! I gave this a (relatively) quick once-over and all the tokens look like they're being used. I called out one issue I noticed in the CSS with border-width and noted in Picker that the spacing is looking a little weird.
I'll plan on circling back to this and looking more closely at your questions on the next round!
components/popover/index.css
Outdated
|
|
||
| /* border-color: var(--spectrum-popover-border-color-default); */ | ||
|
|
||
| border-width: var(--mod-popover-border-width, var(--spectrum-popover-border-thickness)); |
There was a problem hiding this comment.
This mod --mod-popover-border-width is used in a few places, do we want to change the name of it to match its --spectrum-prefixed custom property?
There was a problem hiding this comment.
Sure! Good catch 😄
| --spectrum-popover-background-color: var(--spectrum-background-layer-2-color); | ||
| --spectrum-popover-border-color: var(--spectrum-gray-400); | ||
| --spectrum-popover-background-color: var(--spectrum-background-layer-2-color); | ||
| --spectrum-popover-border-thickness: var(--spectrum-border-width-100); |
There was a problem hiding this comment.
It looks like there are still several references to --spectrum-popover-border-width which is defined in the themes/spectrum.css, this custom prop either needs to be updated to the same name or the references to the old one maybe should be removed and replaced... 🤔 I'm honestly not sure what the best move is here, it might be worth looking over the Foundations PR to get a feel for how we anticipate the different themes will work together in main.
components/popover/index.css
Outdated
| border-radius: var(--mod-popover-corner-radius, var(--spectrum-popover-corner-radius)); | ||
| border-color: var(--highcontrast-popover-border-color, var(--mod-popover-border-color, var(--spectrum-popover-border-color-with-transparency))); | ||
|
|
||
| /* border-color: var(--spectrum-popover-border-color-default); */ |
There was a problem hiding this comment.
A comment here about why this is commented out would be nice, but feel free not to add one if you disagree.
There was a problem hiding this comment.
It was an extraneous comment from earlier edits, trying to popover-border-color-default, which is one of those rgba() tokens.
| @@ -147,11 +149,14 @@ export const Template = ({ | |||
| const popoverMarkup = content.length !== 0 ? Popover({ | |||
| isOpen: isOpen && !isDisabled && !isLoading, | |||
| withTip: false, | |||
| position: "bottom", | |||
| position: "bottom-start", | |||
There was a problem hiding this comment.
Something is looking a little funny with this position in Picker, although changing it back to bottom wouldn't fix it here:
There was a problem hiding this comment.
I think I have that worked out now.
Would this be considered "fragile?" Picker on main has some significant updates that this picker doesn't, so the new CSS in these commits won't really apply with the new picker markup. Maybe I should leave some comments? Very happy for feedback on the fixes either way.
There was a problem hiding this comment.
also, the side label shouldn't be broken now!
Does this apply in this situation? It feels like it's slightly different than that example because this one has an action button? This one for submenu in popover applies the custom styles to Popover to move it, although I'm sure there are probably other ways we could handle it. |
this should make the inline, popover stories show without getting cut off.
- includes code and comments from template.js on main branch - includes code and comments from popover.stories.js on main branch
- utilizes new s2 tokens - add nested popover container margin - adjust tip offset based on new corner radius
- adds the container padding for nested popovers as customStyles - remove inset-inline-start from top/bottom positions this property was causing the popover to be off-centered (likely due to the new corner rounding)
- removes references to spectrum-popover-border-width
- `--bottom-start` was added to the popover within the picker markup - sets --mod-popover-animation-distance to the picker spacing token - adds the popover wrapper div to the CSS selector for the picker's popover
- removes the transform: unset and utilizes a display: block instead for the popover wrapper to override the animation distance properly
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-615-s2-popover
branch
from
b2e341b
to
5e12534
Compare
@rise-erpelding I think you're right, and using the customStyles for popover, within Menu, was what I was thinking. I just wanted to be sure I shouldn't have been touching that. |
- removes `--spectrum-popover-nested-container-spacing` from CSS because it was defined but never used. also removes its reference in the stories file - corrects the margin on the NestedPopover story to use the animation- distance token as margin instead of spacing-100
Description
Welcome to the new and improved S2 popover! 🥳
This PR updates tokens used in the popover component. Additionally, it brings over all of the relevant stories, tests, and documentation from
main(in the hopes of reducing the conflicts the merge may introduce). The copy/pastes frommainare marked with[from main]in the commit messages.- In .storybook/base.css: This update is from
mainand helps to stop cutting off the nested popover content.Most of the work in
popover.stories.jsandtemplate.jsis just copied over frommain.- In template.js:
popoverAlignment["inset-inline-start"] = "0";from 2 position cases (and left a comment explaining that decision).renderContent()frommainwith thecontent.map()that works onspectrum-two.spectrum-twomerges, most of the comments in template.js can be uncommented and refactored to use the approaches we use inmain.- In popover.stories.js`
spectrum-twomerges, most of the comments in popover.stories.js can be uncommented and refactored to use the approaches we use inmain. There's a fewTODOcomments left to hopefully remind us about certain things during that merge.There are several components that have popover as a subcomponent. Those were updated with just enough additional code to make them work. Coachmark did have some additional CSS needs, particularly relating to the new popover corner rounding and the image/media.
Popover Mods
Some mods have been renamed to match their new token names, or better reflect their purposes:
--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-yDesigns
S2 Popover Token specs
S2 / Desktop Popover
Jira
CSS-615
Pending Questions
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
mainappear on the docs page. No immediate changes have been made to popover positions or tip placements.popover-border-color(see the comment in index.css)popover-border-opacitydrop-shadow-elevated-colordrop-shadow-elevated-xdrop-shadow-elevated-ydrop-shadow-elevated-blurcorner-radius-large-defaultpopover-edge-to-content-area(popover-top-to-content-areawas removed from tokens)border-width-100background-layer-2-color(it may not be pointing togray-25as intended, but should once we make the full change to S2 tokens)popover.test.jsfile) continues to maintains test templates, and does not have additional variations/test scenarios.Before:
After:
Regression testing
Validate:
Screenshots
To-do list