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(slider,radio): dir before pseudo element #2744

Merged
merged 3 commits into from
May 22, 2024

Conversation

mdt2
Copy link
Collaborator

@mdt2 mdt2 commented May 10, 2024

Description

Per issue #2703 and #2704, some parsers see :pseudo:dir as invalid, so we've changed it so that the pseudo element comes last :dir :pseudo.

Before, the dist output was:

.spectrum-Slider-handle:before:dir(rtl),
[dir="rtl"] .spectrum-Slider-handle:before

.spectrum-Radio-button:after:dir(rtl),
[dir="rtl"] .spectrum-Radio-button:after

After, with this improvement, the dist output is:

.spectrum-Slider:dir(rtl) .spectrum-Slider-handle:before,
[dir="rtl"] .spectrum-Slider .spectrum-Slider-handle:before {
  transform: translate(50%, -50%);
}

.spectrum-Radio:dir(rtl) .spectrum-Radio-button:after,
[dir="rtl"] .spectrum-Radio .spectrum-Radio-button:after {
  transform: translateX(50%) translateY(-50%);
}

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

Slider

  • Validate that the dist output looks correct after building locally
  • Validate that the slider handle location looks correct in Storybook when RTL mode is on

Radio

  • Validate that the dist output looks correct after building locally
  • Validate that the radio focus looks correct in Storybook when RTL mode is on

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

Copy link

changeset-bot bot commented May 10, 2024

🦋 Changeset detected

Latest commit: 9ccdc7a

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

This PR includes changesets to release 2 packages
Name Type
@spectrum-css/slider Patch
@spectrum-css/radio 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 10, 2024

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

Copy link
Contributor

github-actions bot commented May 10, 2024

File metrics

Summary

Total size: 4.65 MB*
Total change (Δ): ⬆ 0.26 KB (0.01%)

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

Package Size Δ
radio 17.98 KB ⬆ 0.03 KB
slider 32.98 KB ⬆ 0.03 KB

Details

radio

File Head Base Δ
index-base.css 16.91 KB 16.88 KB ⬆ 0.03 KB (0.19%)
index-theme.css 1.65 KB 1.65 KB -
index-vars.css 17.98 KB 17.95 KB ⬆ 0.03 KB (0.17%)
index.css 17.98 KB 17.95 KB ⬆ 0.03 KB (0.17%)
metadata.json 9.30 KB 9.26 KB ⬆ 0.03 KB (0.34%)
themes/express.css 1.33 KB 1.33 KB -
themes/spectrum.css 1.32 KB 1.32 KB -

slider

File Head Base Δ
index-base.css 30.64 KB 30.61 KB ⬆ 0.03 KB (0.11%)
index-theme.css 2.92 KB 2.92 KB -
index-vars.css 32.98 KB 32.95 KB ⬆ 0.03 KB (0.10%)
index.css 32.98 KB 32.95 KB ⬆ 0.03 KB (0.10%)
metadata.json 14.17 KB 14.14 KB ⬆ 0.03 KB (0.23%)
themes/express.css 1.76 KB 1.76 KB -
themes/spectrum.css 1.73 KB 1.73 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.

@mdt2 mdt2 changed the title fix(slider): dir before pseudo element fix(slider,radio): dir before pseudo element May 10, 2024
@mdt2 mdt2 force-pushed the mdt2/css-751-fix-slider-dir branch from 12d601e to 3edb65e Compare May 10, 2024 16:46
@mdt2 mdt2 added ready-for-review run_vrt For use on PRs looking to kick off VRT labels May 10, 2024
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.

Looks good! I validated the changes in the dist output with yarn compare and looked at those two components in Storybook in RTL/LTR.

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.

Can you take a look at the impact of changing the dir selector from querying locally to the component vs. looking to the root of the document? My thought is that if a product has a region of the page localized for example in a language requiring rtl, looking to the root element in the DOM tree will give the wrong result.

components/radio/index.css Outdated Show resolved Hide resolved
@castastrophe castastrophe 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 ready-to-merge labels May 14, 2024
@mdt2 mdt2 force-pushed the mdt2/css-751-fix-slider-dir branch from 3edb65e to fa77908 Compare May 14, 2024 15:59
@mdt2 mdt2 requested a review from castastrophe May 14, 2024 16:31
@castastrophe castastrophe enabled auto-merge (squash) May 22, 2024 18:20
@castastrophe castastrophe merged commit e1ef34f into main May 22, 2024
15 of 22 checks passed
@castastrophe castastrophe deleted the mdt2/css-751-fix-slider-dir branch May 22, 2024 18:26
@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
ready-to-merge 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.

3 participants