-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(styles): update placeholder colors #4799
Conversation
This change updates placeholder color from `$text-03` to `$text-05` to meet the color contrast requirement. This change also replaces manual usage of colors in style rules with the Sass mixin for placeholder `@include placeholder-colors`. Fixes carbon-design-system#4231.
Deploy preview for the-carbon-components ready! Built with commit 163ca36 https://deploy-preview-4799--the-carbon-components.netlify.com |
Deploy preview for carbon-components-react failed. Built with commit 163ca36 https://app.netlify.com/sites/carbon-components-react/deploys/5dfac0864c462e0008622621 |
Deploy preview for carbon-elements ready! Built with commit 163ca36 |
@aagonzales Hey! We talked about adjusting the value of placeholder colors here #4281 (comment) and you said it might be better to just change the value of $text-03 to the value of $text-05, would we still want to do that or is it too late? |
I don't have a quick answer for you. I kind of forget the thread of this decision. I want to look at all this again and make sure we're updating the token correctly. I need to focus on finishing up my pattern work right now and don't want to get distracted so I can take a look at this later this week maybe. |
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.
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.
looks good to me, pending visual review
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.
Talked to @aagonzales about this and there's some confusion; need to re-review this after patterns push.
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.
I have confirmed with Bo Campbell, if the "placeholder" text is actually displaying the format of the input or a prompt then it needs to pass color contrast. If however it is non-informational text (ie an example) then it can remain the low contrast placeholder color text-03
Ok so that are 3 possibilities:
text-01
: option text, 4.5:1 contrast- selects
- drop-downs
text-03
: placeholder low contrast, non-informational text- text input
- text area
text-05
: format text or prompt, 4.5:1 contrast- date picker
- time picker
- search
Side note: we should actually probably figure out how to display the format in date and time picker to be present outside of the field.
@@ -47,7 +47,7 @@ | |||
/// @example @include placeholder-colors; | |||
/// @group global-helpers | |||
@mixin placeholder-colors { | |||
color: $text-03; | |||
color: $text-05; |
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.
This should stay text-03
as it is the official "placeholder" text token.
@@ -51,7 +51,7 @@ | |||
} | |||
|
|||
&::placeholder { | |||
color: $text-05; | |||
@include placeholder-colors; |
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.
Search should be using text-05
since placeholder text is being changed back to text-03
@@ -19,8 +19,7 @@ | |||
@mixin combo-box { | |||
.#{$prefix}--combo-box .#{$prefix}--text-input { | |||
&::placeholder { | |||
color: $text-02; | |||
opacity: 1; | |||
@include placeholder-colors; |
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 be text-05
Thanks for the updated spec @aagonzales - Updated. |
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.
👍
This change updates placeholder color of some components from `$text-03` to e.g. `$text-05` to meet the color contrast requirement. This change also replaces manual usage of colors in style rules with the Sass mixin for placeholder `@include placeholder-colors`. Fixes carbon-design-system#4231.
This change updates placeholder color from
$text-03
to$text-05
to meet the color contrast requirement.This change also replaces manual usage of colors in style rules with the Sass mixin for placeholder
@include placeholder-colors
.Fixes #4231.
Fixes #4225.
Fixes #4254.
Changelog
Changed
$text-05
.@include placeholder-colors
.Testing / Reviewing
Testing should make sure the following components are not broken: