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

Remove autocheck + status-indicator styles #2390

Merged
merged 4 commits into from
Feb 24, 2023
Merged

Remove autocheck + status-indicator styles #2390

merged 4 commits into from
Feb 24, 2023

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Feb 22, 2023

What are you trying to accomplish?

This closes #442 by removing all styles that reference an image asset like url('/images/....

What approach did you choose and why?

  • 🔥 Remove autocheck styles
  • 🔥 Remove status-indicator styles

They will be moved to dotcom with https://github.com/github/github/pull/260973.

What should reviewers focus on?

This is technically a major change, but I marked it as a patch. Reasons to maybe get away with it:

  • The styles that are removed are undocumented and are dependant on JS from dotcom, so I think there is a lower chance that someone has been using them.
  • That issue has been open for many years, so would be good to finally fix it without having to wait for the next major release.

Let me know if you think it's too risky.

Can these changes ship as is?

@changeset-bot
Copy link

changeset-bot bot commented Feb 22, 2023

🦋 Changeset detected

Latest commit: 1c8b98c

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

This PR includes changesets to release 1 package
Name Type
@primer/css 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

@github-actions github-actions bot temporarily deployed to Storybook Preview February 22, 2023 09:08 Inactive
@simurai simurai temporarily deployed to github-pages February 22, 2023 09:08 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to Storybook Preview February 22, 2023 09:09 Inactive
@github-actions github-actions bot temporarily deployed to Storybook Preview February 22, 2023 13:14 Inactive
@simurai simurai marked this pull request as ready for review February 22, 2023 13:28
@simurai simurai requested a review from a team as a code owner February 22, 2023 13:28
Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

These classes aren't used in PVC 🚀

@simurai
Copy link
Contributor Author

simurai commented Feb 24, 2023

These classes aren't used in PVC

Ahh.. good point. I search a bit more in the primer + github orgs. I found one place that seems to use the status-indicator. But that repo is still on "@primer/css": "^15.1.0",. So I assume they are not gonna update anytime soon since it probably would break lots of other things too.

@simurai simurai merged commit 73ae8ec into main Feb 24, 2023
@simurai simurai deleted the remove-autocheck branch February 24, 2023 02:15
@primer-css primer-css mentioned this pull request Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please include images in this project
2 participants