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

[TextField] Fix FilledInput AA contrast issue #25046

Merged

Conversation

Dripcoding
Copy link
Contributor

@Dripcoding Dripcoding commented Feb 21, 2021

Update opacity for light backgroundColor in FilledInput
Update opacity in palette

Fixes #249247

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 21, 2021

Details of bundle changes

Generated by 🚫 dangerJS against eb03da1

@oliviertassinari oliviertassinari changed the title [TextField] fix FilledInput contrast issue [TextField] Fix FilledInput AA contrast issue Feb 21, 2021
@oliviertassinari oliviertassinari added accessibility a11y design: material This is about Material Design, please involve a visual or UX designer in the process labels Feb 21, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Many of the changes are within the approximation threshold of Argos-CI (not reporting changes). Here is an example:

before

before

after

after

I can't think of any concern, it looks like a great change.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I don't think we should change the palette as a whole. If there's a problem with the TextField -> just change the TextField. If there's a problem with the palette -> show the problem with the palette. If that problem only exists with the TextField, don't change the palette since that can have unintended side-effects.

@oliviertassinari
Copy link
Member

If there's a problem with the palette -> show the problem with the palette

@eps1lon Screenshot 2021-02-22 at 16 49 37

https://material.io/design/color/text-legibility.html#text-backgrounds

@oliviertassinari
Copy link
Member

@Dripcoding What do you think about splitting the change into two pull requests? The first one to update the palette, to get a proper entry in the changelog, and this one to polish the filled text field?

@Dripcoding
Copy link
Contributor Author

Dripcoding commented Feb 22, 2021

@oliviertassinari I think splitting this PR makes sense so that each addresses a different issue. I'll split this PR once I get off work today. Also should I create a separate issue for the palette change?

@oliviertassinari
Copy link
Member

Also should I create a separate issue for the palette change?

I don't think so, I think that detailing the motivation for the change is enough.

@Dripcoding
Copy link
Contributor Author

@oliviertassinari @eps1lon I've split this PR with this for the FilledInput.

#25060 is for changing the palette.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Now I get it. Thanks for taking the time explaining both issues!

@oliviertassinari oliviertassinari merged commit 4793d2d into mui:next Feb 23, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 23, 2021

@Dripcoding Perfect, a better aesthetic and a11y (I only hope the background is dark enough to be seen)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TextField] Accessibility contrast issue with filled variant
4 participants