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

chore(storybook): align typography elements for VRTs #3343

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

castastrophe
Copy link
Collaborator

@castastrophe castastrophe commented Oct 28, 2024

Description

Currently VRTs are leveraging the Typography element for the ignored headings. Although they use the "chromatic-ignore" class, this can cause layout issues when the line-height or font-size changes for those typographical elements, leading to unexpected VRT differences. This PR aims to reduce that risk by isolating type styles for VRT from the design system.

In order for the text color to respond to the background context without using a token value, we need to pass the context data through the Container down to the Heading. To this end I've added the context passthroughs in all the existing uses.

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

  • Expect no changes to VRTs
  • Expect headings to always show using "light" mode at the top-level of the testing grids
  • Expect headings inside the testing grids to respond to the global color context settings (white in dark mode, black in light mode)
  • Expect headings on the docs pages to show using "dark" mode when the dark mode styles are applied

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

@castastrophe castastrophe added 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 Oct 28, 2024
@castastrophe castastrophe self-assigned this Oct 28, 2024
Copy link

changeset-bot bot commented Oct 28, 2024

⚠️ No Changeset found

Latest commit: be3640c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Oct 28, 2024

File metrics

Summary

Total size: 4.30 MB*

🎉 No changes detected in any packages

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

Copy link
Contributor

github-actions bot commented Oct 28, 2024

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

@castastrophe castastrophe force-pushed the castastrophe/chore-typography-alignment-vrts branch 5 times, most recently from 9436a14 to 925f091 Compare October 29, 2024 00:15
@castastrophe
Copy link
Collaborator Author

@castastrophe castastrophe added skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review and removed run_vrt For use on PRs looking to kick off VRT labels Oct 29, 2024
Copy link
Member

@cdransf cdransf left a comment

Choose a reason for hiding this comment

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

LGTM! Just had one minor question. ✨

.storybook/decorators/utilities.js Outdated Show resolved Hide resolved
@jawinn jawinn self-requested a review October 29, 2024 18:42
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.

This looks like the same issue you just linked in your comment, but wanted to point it out. The color of the headings on some "Docs" pages was the only issue I saw.

PR: Docs, dark
Screenshot 2024-10-29 at 4 12 55 PM

Prod: Docs, dark
Screenshot 2024-10-29 at 4 12 47 PM

@castastrophe castastrophe force-pushed the castastrophe/chore-typography-alignment-vrts branch from 925f091 to 232b3eb Compare October 30, 2024 13:18
@castastrophe
Copy link
Collaborator Author

@cdransf @jawinn I added a new test case for the docs pages too and corrected the error we were seeing. Thanks!

@castastrophe castastrophe force-pushed the castastrophe/chore-typography-alignment-vrts branch from 232b3eb to 00f560b Compare October 30, 2024 16:38
@jawinn
Copy link
Collaborator

jawinn commented Oct 30, 2024

@cdransf @jawinn I added a new test case for the docs pages too and corrected the error we were seeing. Thanks!

I see dark as fixed. It looks like darkest has the same issue.

@castastrophe castastrophe force-pushed the castastrophe/chore-typography-alignment-vrts branch 2 times, most recently from d9bf6b9 to d7d05af Compare October 30, 2024 21:11
@castastrophe
Copy link
Collaborator Author

@cdransf @jawinn I added a new test case for the docs pages too and corrected the error we were seeing. Thanks!

I see dark as fixed. It looks like darkest has the same issue.

Good call-out @jawinn, updated it to startsWith("dark") instead of === "dark"

@castastrophe castastrophe force-pushed the castastrophe/chore-typography-alignment-vrts branch from d7d05af to be3640c Compare November 1, 2024 16:04
@castastrophe castastrophe enabled auto-merge (squash) November 1, 2024 16:05
@castastrophe castastrophe merged commit 3e6d3ae into main Nov 1, 2024
16 of 24 checks passed
@castastrophe castastrophe deleted the castastrophe/chore-typography-alignment-vrts branch November 1, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. 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