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

fix(list-box): replace value attribute selectors #3411

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Jul 16, 2019

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 fields

Changelog

Changed

  • reset font family for all inputs
  • listbox selectors for populated and empty input fields

Testing / Reviewing

Ensure the listbox components (React only combobox, multiselect, filterable multiselect, and dropdown) display properly

@netlify
Copy link

netlify bot commented Jul 16, 2019

Deploy preview for the-carbon-components ready!

Built with commit d331405

https://deploy-preview-3411--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jul 16, 2019

Deploy preview for carbon-components-react ready!

Built with commit d331405

https://deploy-preview-3411--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Jul 16, 2019

Deploy preview for carbon-elements ready!

Built with commit d331405

https://deploy-preview-3411--carbon-elements.netlify.com

Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

Looks great!

@jeanservaas
Copy link
Contributor

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

  1. the chevron looks a little darker than the close ‘x.’
  2. List box's disabled state text is darker than Combobox's

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

image

List box disabled state

image

@emyarod emyarod force-pushed the 3290-combobox-styling-framework-agnostic branch from f015867 to 5093716 Compare July 22, 2019 13:40
@emyarod
Copy link
Member Author

emyarod commented Jul 22, 2019

@jeanservaas it should be resolved now

@joshblack joshblack requested review from jeanservaas and removed request for jeanservaas July 22, 2019 15:26
@jeanservaas
Copy link
Contributor

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

image

Same on multi-select

image

whereas combobox seems to be consistent:

image

Vanilla

Dropdown is consistent but a darker value than Combobox

image

List box chevron and text is inconsistent value too

image

image

@emyarod emyarod force-pushed the 3290-combobox-styling-framework-agnostic branch from 5093716 to 374fe46 Compare July 23, 2019 12:54
@emyarod
Copy link
Member Author

emyarod commented Jul 23, 2019

@jeanservaas unless I am misunderstanding, this feedback conflicts with #3217 which was closed by #3301 a couple of weeks ago

@jeanservaas
Copy link
Contributor

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.

image

@emyarod
Copy link
Member Author

emyarod commented Sep 30, 2019

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

@vpicone
Copy link
Contributor

vpicone commented Oct 1, 2019

@emyarod agreed, it's broken if a placeholder isn't provided (the default case)

input[type='file'],
input[type='text'],
input[type='password'],
input,
Copy link
Contributor

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>.

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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!

@asudoh asudoh merged commit 1c5d33a into carbon-design-system:master Oct 11, 2019
@emyarod emyarod deleted the 3290-combobox-styling-framework-agnostic branch October 17, 2019 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combo box styling using input[value='...'] is not framework agnostic
10 participants