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.

Sign up for GitHub

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

Jump to bottom

fix(picker): spinner position in RTL direction #2567

Merged
merged 5 commits into from
Mar 15, 2024
Merged

fix(picker): spinner position in RTL direction #2567

merged 5 commits into from
Mar 15, 2024

Conversation

Rocss
Copy link
Contributor

@Rocss Rocss commented Feb 28, 2024 •
edited
Loading

Description

Aims to fix #2566

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

  1. Open the docs for the picker component (where is storybook deployed? 🙈 ):
  • is-loading LTR - margin is applied between the spinner and the label
  • is-loading RTL - margin is applied between the spinner and the label
  • is-invalid LTR - margin is applied between the spinner and the icon
  • is-invalid RTL - margin is applied between the spinner and the icon
  • not invalid - label has no margin end
  • not loading - label has no margin end

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

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 GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 28, 2024 •
edited
Loading

File metrics

Summary

Total size: 3.96 MB*
Total change (Δ): ⬆ 0.19 KB (0.00%)
Table reports on changes to a package's main file. Other changes can be found in the collapsed "Details" below.

Package Size Δ
picker 30.87 KB ⬆ 0.04 KB
Details

picker

File Head Base Δ
index-base.css 28.98 KB 28.94 KB ⬆ 0.04 KB (0.15%)
index-theme.css 2.46 KB 2.46 KB -
index-vars.css 30.87 KB 30.82 KB ⬆ 0.04 KB (0.14%)
index.css 30.87 KB 30.82 KB ⬆ 0.04 KB (0.14%)
mods.json 2.86 KB 2.81 KB ⬆ 0.05 KB (1.81%)
themes/express.css 1.47 KB 1.47 KB -
themes/spectrum.css 1.58 KB 1.58 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.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 28, 2024 •
edited
Loading

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

@Rocss Rocss marked this pull request as ready for review February 28, 2024 10:47
@Rocss Rocss requested a review from castastrophe February 28, 2024 10:47
Rocss
Rocss commented Feb 28, 2024
View reviewed changes
@pfulton pfulton requested review from jawinn and mdt2 February 28, 2024 15:27
@mdt2 mdt2 added the run_vrt For use on PRs looking to kick off VRT label Feb 28, 2024
mdt2
mdt2 reviewed Feb 28, 2024
View reviewed changes
Copy link
Collaborator

@mdt2 mdt2 left a comment

Choose a reason for hiding this comment

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

Visually this change is looking good! Just a couple of comments related to deprecating mods.

To answer your question about finding storybook, in the docs site side navigation, you can scroll down to the bottom and click on "Component preview" and it should pull up storybook.

@castastrophe
Copy link
Collaborator

Thanks @mdt2, great feedback. I have no additional feedback, just +1 to her suggestions.

@Rocss
Copy link
Contributor Author

Rocss commented Feb 29, 2024 •
edited
Loading

One thing I observed, unrelated to this change because it was happening before too, is the size of the warning icon in XL Picker, this seems like a suspicious size:

Screenshot 2024-02-29 at 12 03 05

@mdt2
Copy link
Collaborator

mdt2 commented Feb 29, 2024

One thing I observed, unrelated to this change because it was happening before too, is the size of the warning icon in XL Picker, this seems like a suspicious size:

Screenshot 2024-02-29 at 12 03 05

This does look strange. I took a quick peek and it looks like the token definition matches the S1 design spec, but we have a margin-inline-start value that seems to be limiting it at the XL size:
picker-invalid

@Rocss
Copy link
Contributor Author

Rocss commented Mar 5, 2024

Guys, I have no more things to add to this PR, happy to implement any other feedback, or else we can merge it.
cc: @mdt2 @castastrophe

mdt2
mdt2 approved these changes Mar 11, 2024
View reviewed changes
Copy link
Collaborator

@mdt2 mdt2 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me, thank you! @castastrophe, @pfulton this should be ready for a final look/merge 👍

@pfulton pfulton force-pushed the rocss/adjust-picker-spinner-margins branch 2 times, most recently from 5b8e4eb to 84359f5 Compare March 15, 2024 14:26
@pfulton pfulton 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 Mar 15, 2024
@pfulton pfulton force-pushed the rocss/adjust-picker-spinner-margins branch from 84359f5 to 109beb1 Compare March 15, 2024 14:41
@pfulton pfulton merged commit 770fd5a into main Mar 15, 2024
11 checks passed
@pfulton pfulton deleted the rocss/adjust-picker-spinner-margins branch March 15, 2024 14:58
jawinn pushed a commit that referenced this pull request Mar 15, 2024
@Rocss @jawinn
fix(picker): spinner position in RTL direction (#2567)
7d5199b 
* fix(picker): spinner position in RTL direction

* chore(picker): apply styles only to is-loading and is-invalid

* chore(picker): commit mods.md

* chore(picker): add deprecation

Co-authored-by: Melissa Thompson <[email protected]>

* chore(picker): renamed variable

---------

Co-authored-by: rmanole <[email protected]>
Co-authored-by: Melissa Thompson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdt2 mdt2 mdt2 approved these changes

@castastrophe castastrophe Awaiting requested review from castastrophe

@jawinn jawinn Awaiting requested review from jawinn

Assignees
No one assigned
Labels
skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Picker][RTL] Spinner margins are wrong
4 participants
@Rocss @castastrophe @mdt2 @pfulton