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): rename is-focused class to is-keyboardFocused #2377

Merged
merged 2 commits into from
Jan 17, 2024
Merged

fix(picker): rename is-focused class to is-keyboardFocused #2377

merged 2 commits into from
Jan 17, 2024

Conversation

jawinn
Copy link
Collaborator

@jawinn jawinn commented Dec 18, 2023 •
edited by castastrophe
Loading

Description

This update renames the .is-focused class as it was previously being used as if it were .is-keyboardFocused.
This also fixes a bug in the docs site where the focus indicator was appearing when you clicked the picker twice.

NOTE: This will affect the current spectrum CSS config file for the SWC version of this component, which looks like it will need to change its conversion of this class to an attribute to use .is-keyboardFocused instead.

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

  • Component visuals have not changed for focus and keyboard focus
  • The "Keyboard Focused" story shows the focus indicator ring in Storybook & Chromatic VRT

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 Dec 18, 2023 •
edited
Loading

File metrics

Summary

Total size: 3.92 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.91 KB ⬆ 0.06 KB
Details

picker

File Head Base Δ
index-base.css 29.02 KB 28.96 KB ⬆ 0.06 KB (0.22%)
index-theme.css 2.46 KB 2.46 KB -
index-vars.css 30.91 KB 30.84 KB ⬆ 0.06 KB (0.20%)
index.css 30.91 KB 30.84 KB ⬆ 0.06 KB (0.20%)
mods.json 2.81 KB 2.81 KB -
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 Dec 18, 2023 •
edited
Loading

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

@jawinn jawinn marked this pull request as ready for review December 18, 2023 20:27
mdt2
mdt2 approved these changes Dec 18, 2023
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.

This looks good! Probably should run VRTs before merging.

@jawinn jawinn added the run_vrt For use on PRs looking to kick off VRT label Jan 5, 2024
@jawinn jawinn force-pushed the jawinn/picker-remove-is-focused branch 2 times, most recently from 9f461bc to f86f46a Compare January 5, 2024 20:15
@castastrophe castastrophe force-pushed the jawinn/picker-remove-is-focused branch from f86f46a to 7472b6a Compare January 5, 2024 21:38
@castastrophe castastrophe added size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. ready-for-review labels Jan 5, 2024
@jawinn jawinn force-pushed the jawinn/picker-remove-is-focused branch from 7472b6a to 805b974 Compare January 8, 2024 16:01
@jawinn jawinn changed the title fix(picker): remove is-focused class which was used as key focus fix(picker): rename is-focused class to is-keyboardFocused Jan 8, 2024
@jawinn
Copy link
Collaborator Author

jawinn commented Jan 8, 2024

After looking at how SWC is converting this, I've decided not to remove the class and have renamed it to .is-keyboardFocused instead.

@jawinn jawinn requested a review from mdt2 January 8, 2024 16:06
@jawinn jawinn force-pushed the jawinn/picker-remove-is-focused branch 3 times, most recently from 3bd31c6 to db01584 Compare January 8, 2024 16:49
@castastrophe castastrophe force-pushed the jawinn/picker-remove-is-focused branch from db01584 to 2d57d6e Compare January 17, 2024 14:14
@castastrophe castastrophe enabled auto-merge (squash) January 17, 2024 14:16
@castastrophe castastrophe force-pushed the jawinn/picker-remove-is-focused branch from 2d57d6e to 7ea959f Compare January 17, 2024 14:28
jawinn added 2 commits January 17, 2024 11:31
@jawinn @castastrophe
fix(picker): key focus styles should not be used for focus class
b2a40e9 
The .is-focused class was being used as if it was .is-keyboardFocused,
in combination with :focus-visible in selectors. This class has been
removed as it is handled by :focus-visible on the button.

This fixes an existing issue on the docs site, where if you click the
picker twice, the focus indicator ring will appear. This is because of
the docs site adding the .is-focused class.

Updated the existing story that was labeled "Focused" that was showing
keyboard focused to use the play function, for Chromatic coverage
ref: https://www.chromatic.com/docs/hoverfocus/
@jawinn @castastrophe
fix(picker): rename is-focused to is-keyboardFocused
5a5925b 
Rather than removing the class, keep it in its renamed form in order
for SWC to still be able to convert it to an attribute and continue
testing it via its Storybook control.
@castastrophe castastrophe force-pushed the jawinn/picker-remove-is-focused branch from 7ea959f to 5a5925b Compare January 17, 2024 16:31
@castastrophe castastrophe merged commit 60b44bb into main Jan 17, 2024
10 checks passed
@castastrophe castastrophe deleted the jawinn/picker-remove-is-focused branch January 17, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@castastrophe castastrophe castastrophe approved these changes

@mdt2 mdt2 Awaiting requested review from mdt2

Assignees
No one assigned
Labels
ready-for-review run_vrt For use on PRs looking to kick off VRT size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jawinn @castastrophe @mdt2