-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiComboBox] Matching placeholder color to other form elements #4744
Conversation
@cchaos In #4703 you mentioned making Let me know if there's anything else that needs to be done to it. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4744/ |
Yeah sorry, I didn't mean that |
@@ -16,7 +16,6 @@ | |||
@include euiTextBreakWord; /* 2 */ | |||
padding: $euiSizeS; | |||
text-align: center; | |||
color: $euiColorDarkShade; |
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.
While looking through all the instances of $euiColorDarkShade
it seemed like this one never did anything. (Directly inside of this class is EuiText
which overrides the color again so this color is never used.)
Open considerations:
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4744/ |
Here's how we think about global vs component level variables. Global Component level The idea behind my suggestion is that we have a huge list of form specific colors, sizing, etc, variables. Like that variable above
What you are trying to achieve is a consistent color across our form placeholder text colors. We previously, were manually using a shade color to apply placeholder text color in a mixin, but that mixin wasn't being used by all form inputs. eui/src/global_styling/mixins/_form.scss Lines 85 to 87 in e6fd955
So my suggestion is, if you can't use the mixin, we should ensure we never get out of sync with our placeholder text colors by creating, specifically, a form-level variable to be added to that $euiFormControlPlaceholderColor: $euiColorDarkShade !default; Sorry for the verbosness, but I wanted to take this as an opportunity to document systems thinking (specifically how we've done in historically within EUI). And I think it may be helpful to others more than just this PR specifically. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4744/ |
Co-authored-by: Caroline Horn <[email protected]>
Preview documentation changes for this PR: https://eui.elastic.co/pr_4744/ |
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.
The new variable is 💯 . I tested in Chrome, Safari, and Firefox. Firefox seems to have an opacity problem...
7987fd1
to
d976f03
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_4744/ |
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 great to me. Firefox is fixed now and they all have the same color. Even the dark mode text is a better color too.
Preview documentation changes for this PR: https://eui.elastic.co/pr_4744/ |
Co-authored-by: Caroline Horn <[email protected]>
Looks good on both firefox and chrome. They both have the same color for the placeholder text (#69707D) and the input field's background (#FBFCFD) with a contrast ratio of 4.9:1. I looked at https://eui.elastic.co/pr_4744/#/forms/form-controls Nice job. I bumped up the font to 500% to make sure I snagged the right color in a vertical letter so as not to select an anti-aliased color |
Summary
Closes #4703
ComobBox placeholder color was too low to follow WCAG guidelines and didn't match our other input placeholders.
Created
$euiFormControlPlaceholderText
and updated forms and combobox to use the new variable.Checklist
[ ] Checked in mobile[ ] Props have proper autodocs and playground toggles[ ] Added documentation[ ] Checked Code Sandbox works for the any docs examples[ ] Added or updated jest tests