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(colorloupe): adds correct disabled styles #3453

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marissahuysentruyt
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt commented Dec 18, 2024

Description

It was noted in #3409 that the color loupe shouldn't display when its color handle is disabled. If a user went to color handle, turned the withColorLoupe to true, then tried to disable the color handle, and the color loupe is still rendering. This PR aims to fix that by adding styles for disabled color loupes. There's also the corresponding changeset and udpates to the metadata.json.

Before 🚫 (color loupe testing grid/color handle testing grid)
Screenshot 2025-01-03 at 12 41 38 PM
Screenshot 2025-01-03 at 3 14 02 PM

After ✅ (color loupe testing grid/color handle testing grid)
Screenshot 2025-01-03 at 12 38 04 PM
Screenshot 2025-01-03 at 12 37 49 PM

Jira/Specs

CSS-1089

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

  • Pull down the branch to run locally or visit the deploy preview. [@castastrophe]
    • Go to the color loupe default story
    • Toggle the disabled control to verify the color loupe isn't visible. ((it should have opacity: 0 in the inspector)
    • Turn on the Chromatic preview (and if needed reset the color loupe controls)
  • Verify the Disabled test case behaves the same way and isn't visible. [@castastrophe]
  • Go to the color handle default story
    • Turn on both the disabled and withColorLoupe controls. Toggle the disabled control a few times.
    • Verify the color loupe only displays when the color handle is not disabled. [@castastrophe]
    • Turn on the Chromatic preview (and if needed reset the color handle controls)
  • Verify the With Color Loupe- Disabled test case behaves the same and the color loupe isn't visible. [@castastrophe]

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.
  • If my change impacts other components, I have tested to make sure they don't break.
  • ✨ This pull request is ready to merge. ✨

Copy link

changeset-bot bot commented Dec 18, 2024

🦋 Changeset detected

Latest commit: 2ecf9c0

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/colorloupe Minor

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

@marissahuysentruyt marissahuysentruyt self-assigned this Dec 18, 2024
Copy link
Contributor

github-actions bot commented Dec 18, 2024

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

Copy link
Contributor

github-actions bot commented Dec 18, 2024

File metrics

Summary

Total size: 4.27 MB*
Total change (Δ): 🔴 ⬆ 0.15 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 Δ
colorloupe 3.95 KB 🔴 ⬆ 0.05 KB

Details

colorloupe

Filename Head Compared to base
index-base.css 3.95 KB 🔴 ⬆ 0.05 KB (1.25%)
index-vars.css 3.95 KB 🔴 ⬆ 0.05 KB (1.25%)
index.css 3.95 KB 🔴 ⬆ 0.05 KB (1.25%)
* 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.

- correctly adds styles for disabled color loupes
- rebuilds metadata.json
- create changeset
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-disabled-color-loupe branch from b5a996e to 2ecf9c0 Compare January 3, 2025 19:43
@marissahuysentruyt marissahuysentruyt added bug Results from a bug in the CSS implementation size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. run_vrt For use on PRs looking to kick off VRT labels Jan 3, 2025
@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review January 6, 2025 18:04
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.

Ran through the test cases and they're looking great! I am still seeing the box-shadow when activating the disabled knob but I think that's because the object maintains focus and clicking away hides the shadow. I think it's a Storybook-only issue and not necessarily something we need to fix.

@rise-erpelding rise-erpelding self-requested a review January 6, 2025 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Results from a bug in the CSS implementation 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
Development

Successfully merging this pull request may close these issues.

3 participants