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

Styled component bug cleanup #3540

Merged
merged 5 commits into from
Jul 27, 2023
Merged

Conversation

kendallgassner
Copy link
Contributor

While working on Stylelint-a11y spike/exploration I noticed a couple of bugs in some styled blocks. This pr fixes those bugs -- they are pretty much nits.

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@kendallgassner kendallgassner requested review from a team and mperrotti July 18, 2023 20:49
@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2023

🦋 Changeset detected

Latest commit: 0867c26

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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

@kendallgassner kendallgassner requested a review from TylerJDev July 18, 2023 20:49
@github-actions
Copy link
Contributor

github-actions bot commented Jul 18, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 102.42 KB (-0.01% 🔽)
dist/browser.umd.js 103 KB (-0.01% 🔽)

@kendallgassner kendallgassner temporarily deployed to github-pages July 18, 2023 20:55 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3540 July 18, 2023 20:55 Inactive
@kendallgassner kendallgassner temporarily deployed to github-pages July 18, 2023 21:19 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3540 July 18, 2023 21:20 Inactive
@kendallgassner kendallgassner temporarily deployed to github-pages July 18, 2023 21:30 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3540 July 18, 2023 21:31 Inactive
@kendallgassner kendallgassner temporarily deployed to github-pages July 19, 2023 15:35 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3540 July 19, 2023 15:35 Inactive
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Thanks for this! 🥳

@joshblack
Copy link
Member

joshblack commented Jul 19, 2023

One note for @mperrotti, it seems like the change here updates some styling for Timeline across light and dark high contrast themes. Specifically, it seems like the line above the item is not being displayed:

A GitHub timeline that shows two messages. There is a vertical bar that connects the status icons of these messages but the bar is missing in the second message in the timeline

I assume that the intent here is for the line above the second item to be displayed? Just wanted to double-check 👀

@kendallgassner
Copy link
Contributor Author

I'm mostly just waiting to merge this because of the comment above. @joshblack if you don't think waiting is necessary feel free to merge 🙏 .

@mperrotti
Copy link
Contributor

@joshblack - these changes don't look like they should update how any of the styles are rendered. If the Timeline looks different on main, we should consider those to be visual regressions.

@joshblack
Copy link
Member

Thanks @mperrotti!

@kendallgassner should be good to go! Thanks again for this, I just added the update snapshots which should trigger updating the snapshots so VRT goes green ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants