fix(popover): offset matches spec

[mdt2]

Description

This has been a difficult component to test without implementing it. What I landed on is that the 8px offset distance in the spec should be accomplished by the animation.

For popovers without a tip, we can simply customize the animation-distance value to align with the spec.

For popovers with a tip, we need to add an offset margin equal to the width or height of the tip (depending on the position of the popover) so that the animation-distance is applied against the popover + tip.

• Popover (or it's tip if there is one) is offset 8px from the source per the spec
• Docs have been updated to reflect the change to default
• Storybook examples have been updated to show the correct default offset

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

Tip for Docs Site: If you're having trouble seeing if the animation is actually moving 8px like I was, you can turn off the visibility changes in components/commons/overlay-coretokens.css by changing visibility: hidden to visibility: visible and opacity: 0; to opacity: 1;. This will let you see the popover move up and down 8px on click.

Note about positioning: We're forcing position of the popover in both the Docs site and in Storybook, but we do it in different ways on each. The Docs site uses a dummy-spacing class on a div that we can put right up next to the dummy source which gives the popover the space to move the 8px animation-distance on open/close. In Storybook there are some extra calculations. I've adjusted the offset value for each situation to give it 8px distance visually, but the values applied here weren't a uniform solution.

• Popover is 8px from the source at all positions (you'll have to close between changing positions or you'll see weird behavior)
  • Default Storybook example
    • LTR
    • RTL
  • With Tip Storybook example
    • LTR
    • RTL
  • Nested Storybook example
    • LTR
    • RTL
• Docs site examples show popover 8px from source and they animate in and out @jenndiaz

Regression testing

Validate:

1. A legacy documentation page (such as accordion), including:

• The page renders correctly
• The page is accessible
• The page is responsive

2. A migrated documentation page (such as action group), including:

• The page renders correctly
• The page is accessible
• The page is responsive

Screenshots

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. ✨

[github-actions]

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

[jenndiaz]

The docs site looks good. I had some questions about the storybook with tip values.

[jenndiaz]

looks good to me!

[jawinn]

Won't this immediately get overwritten by the same variable that's defined in the %spectrum-overlay--top--open? Also, I'm wondering if it makes more sense to just use the one transform line it's importing with new values rather than needing the @inherit and needing to move the custom property after the @inherit.

Initially I was thinking that this variable could be defined once in .spectrum-Popover.is-open since it appears to be defined the same way for is-open on all (?) the variants. Or rather in .spectrum-Popover--it looks like %spectrum-overlay is adding the animation distance of 6px (that has a TODO on it to replace with core tokens...), so that would have to be after the @inherit: %spectrum-overlay;.

[mdt2]

Yeah, per that convo in the tooltip PR (#2266), I added a commit that reworks this to define the transforms in popover/index.css instead 👍 Shouldn't change any functionality, just makes the code more readable and DRY.

[github-actions]

File metrics

Overall Δ: +4.44 KB ⬆ (+0.12%)

popover

+4.44 KB ⬆

File	Size	Diff	Δ	Δ%
Total changes	92.38 KB	96.81 KB	+4.44 KB ⬆	+4.80%
index-base.css	30.03 KB	31.51 KB	+1.47 KB ⬆	+4.91%
index-theme.css	< 1KB	< 1KB	No change 🎉	0%
index-vars.css	30.19 KB	31.66 KB	+1.47 KB ⬆	+4.89%
index.css	30.19 KB	31.66 KB	+1.47 KB ⬆	+4.89%
mods.json	< 1KB	< 1KB	+< 1KB ⬆	+2.30%
themes/express.css	< 1KB	< 1KB	No change 🎉	0%
themes/spectrum.css	< 1KB	< 1KB	No change 🎉	0%

[castastrophe]

@mdt2 When you get a chance, can you rebase this one and resolve the conflicts?