Skip to content

fix(stepper): no longer touches InfieldButton classes #2300

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

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

mdt2
Copy link
Collaborator

@mdt2 mdt2 commented Nov 14, 2023

Description

Per issue #2234, Stepper shouldn't be using classes that it doesn't own. This PR adds some properties to InfieldButton and uses their mods to modify them for Stepper so that we no longer touch InfieldButton classes inside our Stepper component. There should be no visual differences to the components.

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

Note: When reviewing the code, filter commits so that you aren't reviewing separate changes made in PR #2234.

  1. Stepper variants match prod:
  • on the docs site in every state (focus, hover, active, keyboard focus)
  • in Storybook in every state (focus, hover, active, keyboard focus)
  1. InfieldButton variants match prod:
  • on the docs site in every state (focus, hover, active, keyboard focus)
  • in Storybook in every state (focus, hover, active, keyboard focus)
  1. The naming of the custom props in the code:
  • makes sense
  • aligns with existing conventions

Tested by @jawinn

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.

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

@mdt2 mdt2 added the run_vrt For use on PRs looking to kick off VRT label Nov 14, 2023
Copy link
Contributor

github-actions bot commented Nov 14, 2023

File metrics

Summary

Total size: 4.06 MB*
Total change (Δ): ⬇ 1.25 KB (0.03%)

Package Size Δ
infieldbutton 58.83 KB ⬇ 1.72 KB
stepper 68.26 KB ⬆ 0.48 KB
Details

infieldbutton

File Size Base Δ
Total 60.55 KB 58.83 KB ⬇ 1.72 KB (2.84%)
index-base.css 16.22 KB 15.71 KB ⬇ 0.53 KB (3.17%)
index-theme.css 2.72 KB 2.72 KB - (no change)
index-vars.css 18.37 KB 17.85 KB ⬇ 0.53 KB (2.80%)
index.css 18.37 KB 17.85 KB ⬇ 0.53 KB (2.80%)
mods.json 2.16 KB 1.98 KB ⬇ 0.18 KB (8.16%)
themes/express.css 1.64 KB 1.64 KB - (no change)
themes/spectrum.css 1.08 KB 1.08 KB - (no change)

stepper

File Size Base Δ
Total 67.79 KB 68.26 KB ⬆ 0.48 KB (0.69%)
index-base.css 17.22 KB 17.55 KB ⬆ 0.34 KB (1.92%)
index-theme.css 3.98 KB 3.85 KB ⬇ 0.13 KB (3.27%)
index-vars.css 20.62 KB 20.82 KB ⬆ 0.21 KB (0.98%)
index.css 20.62 KB 20.82 KB ⬆ 0.21 KB (0.98%)
mods.json 1.38 KB 1.38 KB ⬇ 0.01 KB (0.35%)
themes/express.css 2.30 KB 2.25 KB ⬇ 0.05 KB (2.17%)
themes/spectrum.css 1.67 KB 1.59 KB ⬇ 0.08 KB (4.78%)

* An ASCII character in UTF-8 is 8 bits or 1 byte.

Copy link
Contributor

github-actions bot commented Nov 14, 2023

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

@mdt2 mdt2 removed the run_vrt For use on PRs looking to kick off VRT label Nov 14, 2023
@mdt2 mdt2 force-pushed the mdt2/css-630-stepper-component-separation branch from 95d2f20 to 3596b3c Compare November 14, 2023 18:31
@mdt2 mdt2 changed the title WIP fix(stepper): no longer touches InfieldButton classes fix(stepper): no longer touches InfieldButton classes Nov 14, 2023
@mdt2 mdt2 marked this pull request as ready for review November 14, 2023 18:38
@mdt2 mdt2 requested review from pfulton, jawinn and jenndiaz November 14, 2023 18:42
@mdt2 mdt2 force-pushed the mdt2/css-630-stepper-component-separation branch from c8dfb99 to b297a7a Compare November 16, 2023 19:26
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 to me! I just have one question about two of the newly defined custom properties on .spectrum-InfieldButton that may not be needed.

@mdt2 mdt2 force-pushed the mdt2/css-630-stepper-component-separation branch from b297a7a to 4dc140f Compare November 21, 2023 15:56
@mdt2 mdt2 requested a review from jawinn November 21, 2023 16:03
@pfulton pfulton linked an issue Dec 4, 2023 that may be closed by this pull request
@mdt2 mdt2 force-pushed the mdt2/css-630-stepper-component-separation branch 2 times, most recently from 4b204bf to 4f76b8b Compare December 11, 2023 15:28
@pfulton pfulton added the run_vrt For use on PRs looking to kick off VRT label Dec 11, 2023
@mdt2 mdt2 mentioned this pull request Dec 11, 2023
14 tasks
@pfulton pfulton force-pushed the mdt2/css-630-stepper-component-separation branch from 4f76b8b to dbc7e3d Compare December 11, 2023 21:01
@pfulton pfulton merged commit a82b8ad into main Dec 11, 2023
@pfulton pfulton deleted the mdt2/css-630-stepper-component-separation branch December 11, 2023 21:18
jenndiaz pushed a commit that referenced this pull request Dec 12, 2023
* fix(stepper): no longer touches InfieldButton classes
* fix(stepper): button size + space match infieldbutton spec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run_vrt For use on PRs looking to kick off VRT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Stepper]: Selectors reach into classes/element that it does not own
3 participants