fix(locale-modal): replaced placeholder color for a bolder one #4896
Conversation
|
Deploy preview created for package Built with commit: ccecec97568adab6203ec9cb0effda2c99c2fb97 |
|
Deploy preview created for package Built with commit: ccecec97568adab6203ec9cb0effda2c99c2fb97 |
|
Deploy preview created for package Built with commit: ccecec97568adab6203ec9cb0effda2c99c2fb97 |
There was a problem hiding this comment.
LGTM basically, can we add a comment referring to https://github.com/carbon-design-system/carbon/blob/v10.25.0/packages/components/src/globals/scss/_helper-mixins.scss#L44-L51 and say that we are deliberately diverge from that, and the reason why we do so? (Background; People tend to wonder why we are defining one-off placeholder color here) Thanks @IgnacioBecerra!
jeffchew
added
the
Ready to merge
|
@IgnacioBecerra Is this something we can propose to the Carbon repo? IMO, we should be avoiding making overrides to core components that we have to maintain. I did a little digging in the Carbon repo and found this issue where they updated the placeholder mixins to address accessibility issues with the place holder text. carbon-design-system/carbon#6914 |
|
@oliviaflory Interesting point, thank you for bringing this up - I believe the original code used |
|
@asudoh @oliviaflory Yeah I think that's correct, the placeholder was |
|
@IgnacioBecerra Would you please check the color contrast of the original code to see if it really misses our color contrast standard? I think latest Chrome DevTools has a feature that shows the color contrast. |
|
@asudoh I tried using the color contrast tool but it appears that it doesn't appear to be working. However, I found this website and got these results: Looks like it passes the WCAG AA test, but fails on AAA. However, using cc: @oliviaflory |
|
Thank you @IgnacioBecerra, I didn't realize we were having to pass WCAG AAA, I was under the impression we only needed AA. Might be good to confirm with @Praveen-sr if this is the new standard we should be aware of. |
|
@IgnacioBecerra The associated issue (#4802) seems to say that the pseudo element's color in before-change code was |
|
@asudoh Yeah that's correct, this is the original placeholder using This is with Just as an extra, this is with the current color I thought they had mistakenly copied |
|
I see, does it mean that the issue was never reproducible? Is it worth checking with QA team? |
…n-design-system#4896) ### Related Ticket(s) carbon-design-system#4802 ### Description The previous placeholder color for the Web Components version was failing contrast ratio testing. Changed the placeholder text to match the `$text-02` token used in the React version, where the contrast ratio has been fixed. ### Changelog **Changed** - `<dds-search>` placeholder color changed to `$text-02` to match React version <!-- React and Web Component deploy previews are enabled by default. --> <!-- To enable additional available deploy previews, apply the following --> <!-- labels for the corresponding package: --> <!-- *** "package: services": Services --> <!-- *** "package: utilities": Utilities --> <!-- *** "package: styles": Carbon Expressive --> <!-- *** "RTL": React / Web Components (RTL) --> <!-- *** "feature flag": React / Web Components (experimental) -->
Related Ticket(s)
#4802
Description
The previous placeholder color for the Web Components version was failing contrast ratio testing. Changed the placeholder text to match the
$text-02token used in the React version, where the contrast ratio has been fixed.Changelog
Changed
<dds-search>placeholder color changed to$text-02to match React version