-
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(list-box): replace value
attribute selectors
#3411
fix(list-box): replace value
attribute selectors
#3411
Conversation
Deploy preview for the-carbon-components ready! Built with commit d331405 https://deploy-preview-3411--the-carbon-components.netlify.com |
Deploy preview for carbon-components-react ready! Built with commit d331405 https://deploy-preview-3411--carbon-components-react.netlify.com |
Deploy preview for carbon-elements ready! Built with commit d331405 |
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!
Looks good @emyarod -- just one thing I noticed about disabled state values in both Vanilla and React Something's not right with the disabled state color values. Our color values are not consistent in our disabled states… I'm showing Vanilla examples but the same inconsistency is happening in React Combobox, Multiselect and Dropdown
Everything on a $disabled-01 field should be getting the $disabled-02 token in this situation (so everything should be #bebebe) Combo box disabled state List box disabled state |
f015867
to
5093716
Compare
@jeanservaas it should be resolved now |
hey @emyarod (i know you'd never interact with a disabled dropdown) but the knob let me do this in react and I noticed still the disabled text colors appear to be different and again, the disabled chevron is pulling a lighter value. These are all React in Firefox btw Same on multi-select whereas combobox seems to be consistent: Vanilla Dropdown is consistent but a darker value than Combobox List box chevron and text is inconsistent value too |
5093716
to
374fe46
Compare
@jeanservaas unless I am misunderstanding, this feedback conflicts with #3217 which was closed by #3301 a couple of weeks ago |
374fe46
to
905fb97
Compare
Hey @emyarod yeah it gets a little inception-y. I'm going to call @shixiedesign (since she'll be back) in on this because I'm not the greatest with tokens. Also, something may have changed or our guidance might be off. But this is what I think is happening. When an input field is not in any kind of container and it is disabled, I believe it gets $disabled-01 (which is actually the same as $field-01, we don't use a different value for disabled fields). Anything on top of $disabled-01 (i.e. icons, text) gets $disabled-2. If an input field is on a form or in some kind of container say in the dark theme, if the $ui-background is gray 100 and the container is gray 90, then the input field will be gray 80. A disabled field within a container (gulp, i think) will get $disabled-02 and then the disabled text or an icon on top of that will get $disabled-03. So in this issue, I'm referencing React and Vanilla input fields in the white theme. Here I believe that the fields should be $disabled-01 (which is gray 10, f3f3f3) and the disabled text and icons should get $disabled-02 (which is gray 30, bebebe). If these fields were $disabled-02, which is a required background for a $disabled-03 token, they'd be gray 30 -- that would not be correct. The big point though is that the disabled values of the text and icon are in many cases different and they are inconsistent throughout the dropdowns. Incorrect or correct, the first issue is to make them the same value. There's no reason a disabled icon and disabled text in the same field should be pulling a different value. But we'll talk to shix when she's back. |
f8c0334
to
3d90e7b
Compare
in that case I think we should refactor existing usage of the selector in our library as well like https://github.com/carbon-design-system/carbon/blob/master/packages/components/src/components/data-table/_data-table-action.scss#L155 |
@emyarod agreed, it's broken if a placeholder isn't provided (the default case) |
input[type='file'], | ||
input[type='text'], | ||
input[type='password'], | ||
input, |
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.
@emyarod Is this intentional? Asking because it seems to change the specificity of the reset style for <input>
.
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.
@asudoh yes, in order to override browser user agent styles
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 see, is it needed to complete this PR or was it just an issue you found?
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.
since this is only a style change it's not needed to resolve the original issue but we will need to make this change so that the component uses the correct font
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 sounds fair enough - Thanks for clarifying!
Closes #3290
This PR replaces
value
attribute selectors with:placeholder-shown
selectors in the listbox styles to accommodate frameworks that do not mirror the value attribute and property. This PR also correctly applies the Plex font to all input fieldsChangelog
Changed
input
sTesting / Reviewing
Ensure the listbox components (React only combobox, multiselect, filterable multiselect, and dropdown) display properly