-
Notifications
You must be signed in to change notification settings - Fork 535
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
Change disabled color from muted to custom primer disabled color #1512
Conversation
🦋 Changeset detectedLatest commit: 53fdc9b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
@pksjce These CI are caused by breaking changes in Primer Primitives v5. I'm going to open a separate PR to fix these errors. |
Here's a draft of the PR to update primitives: #1514 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include these two components as well:
- Disabled item for the Pagination:
react/src/Pagination/Pagination.tsx
Line 58 in c156b07
color: ${get('colors.fg.muted')}; // check - Disabled TextInput:
react/src/_TextInputWrapper.tsx
Line 82 in c156b07
color: ${get('colors.fg.muted')}; ::placeholder
as well? Here the change in Primer CSS
@simurai - Updated the color on a bunch of more places! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the color on a bunch of more places!
Ohh.. that might be a few too many. The colors.primer.fg.disabled
isn't meant for replacing all of colors.fg.muted
and only in situations when a component has a disabled state. For example when adding the disabled
attribute to a <button>
.
When using colors.primer.fg.disabled
for the "normal" state, for example on the BranchName
component, then the text might be too subtle and hard to read:
Before | After |
---|---|
colors.fg.muted |
colors.primer.fg.disabled |
So I think we should revert 57b66e3 again and only additionally replace it for disabled Pagination and TextInput: #1512 (review)
Switched muted to new disabled color Update snapshot tests
Thanks @simurai for that review! I have made the appropriate changes. |
Co-authored-by: simurai <simurai@github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Ok, this looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
Summary 👻
Update
primer/primitives
to5.1.0
.Start using
colors.primer.fg.disabled
instead ofcolors.fg.muted
.Context in https://github.com/github/primer/issues/292
Similar change in
primer/css
in primer/css#1677Before n After ✨