Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(popover): correct left/right tips for RTL, increase VRT coverage #2753

Merged
merged 5 commits into from
May 22, 2024

Conversation

mdt2
Copy link
Collaborator

@mdt2 mdt2 commented May 13, 2024

Description

Fixes an issue in Popover where Left and Right tips were mirrored incorrectly for RTL. This is simliar to some work done on Tooltip. Note that the Left and Right placement for the tips are always on the LTR left and right, even if RTL is turned on. Start and End, however, are adjusted based on the text direction.

Also included Storybook enhancements including:

  • Show all placement options (previously Start and End placements weren't included in the Storybook controls)
  • Increase Chromatic VRT coverage for the tip variant

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

  • VRTs passing @mdt2
  • Check Popover With Tip's new testing preview in Storybook. All the tips should look correctly positioned in LTR @jawinn
  • Now check RTL, tips should be correctly positioned
  • All placement options are shown in the Storybook controls

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. 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
before-popover

After
after-popover

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

Copy link

changeset-bot bot commented May 13, 2024

🦋 Changeset detected

Latest commit: fe34cdf

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

This PR includes changesets to release 1 package
Name Type
@spectrum-css/popover Patch

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

Copy link
Contributor

github-actions bot commented May 13, 2024

File metrics

Summary

Total size: 4.65 MB*
Total change (Δ): ⬇ 0.03 KB (-0.00%)

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

Package Size Δ
popover 15.92 KB ⬇ 0.01 KB

Details

popover

File Head Base Δ
index-base.css 15.77 KB 15.78 KB ⬇ 0.01 KB (-0.06%)
index-theme.css 0.75 KB 0.75 KB -
index-vars.css 15.92 KB 15.93 KB ⬇ 0.01 KB (-0.06%)
index.css 15.92 KB 15.93 KB ⬇ 0.01 KB (-0.06%)
metadata.json 10.32 KB 10.32 KB -
themes/express.css 0.66 KB 0.66 KB -
themes/spectrum.css 0.68 KB 0.68 KB -
* 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.

Copy link
Contributor

github-actions bot commented May 13, 2024

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

@mdt2 mdt2 added the run_vrt For use on PRs looking to kick off VRT label May 13, 2024
@mdt2 mdt2 force-pushed the mdt2/css-757-popover-tip-rtl branch 7 times, most recently from fd2c609 to 0b351b9 Compare May 13, 2024 18:38
Copy link
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work fixing this and adding the Storybook improvements. The Chromatic-only view with descriptions makes it much easier to understand which ones should be flipping.

I only have one minor grammar suggestion.

.changeset/three-geese-relax.md Show resolved Hide resolved
components/popover/stories/popover.stories.js Outdated Show resolved Hide resolved
@mdt2 mdt2 force-pushed the mdt2/css-757-popover-tip-rtl branch from 464a104 to 80be570 Compare May 14, 2024 15:52
@mdt2 mdt2 force-pushed the mdt2/css-757-popover-tip-rtl branch 3 times, most recently from b12e278 to 0325b5a Compare May 16, 2024 13:51
@mdt2 mdt2 added skip_vrt Add to a PR to skip running VRT (but still pass the action) and removed run_vrt For use on PRs looking to kick off VRT labels May 16, 2024
Copy link
Collaborator

@castastrophe castastrophe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, just a question about the goals of the sourceless template. I'm wondering if we have to update the class list or add new features, is it going to be hard to remember to update that in both templates? Is there a way to have 1 be a "source of truth" without needing the duplication?

components/popover/index.css Show resolved Hide resolved
@@ -159,3 +171,39 @@ export const Template = ({
</div>
`;
};

export const SourcelessTemplate = ({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some more context in a comment here about why this additional template is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great callout on this! Turns out all I had to do was pass a null trigger where I didn't want a source button to show up (for Chromatic), so I've removed this code 👍

@mdt2 mdt2 force-pushed the mdt2/css-757-popover-tip-rtl branch from a2786f4 to fe34cdf Compare May 22, 2024 19:39
@castastrophe castastrophe merged commit 54faea2 into main May 22, 2024
11 checks passed
@castastrophe castastrophe deleted the mdt2/css-757-popover-tip-rtl branch May 22, 2024 20:13
@github-actions github-actions bot mentioned this pull request May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants