fix(Dropdown): rely on list-box height closes #4916 #4917
Conversation
ghost
requested review from
|
Deploy preview for the-carbon-components ready! Built with commit 5e4da0b https://deploy-preview-4917--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit 5e4da0b https://deploy-preview-4917--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements failed. Built with commit 5e4da0b https://app.netlify.com/sites/carbon-elements/deploys/5dfac41ab505d300084daa15 |
|
@alisonjoseph vanilla is actively making our code worse, someone put it out of its misery. |
There was a problem hiding this comment.
Thank you for jumping in @vpicone - Vanilla dropdown doesn't have .bx--list-box, which will be more solid distinction than detecting ARIA role (given vanilla dropdown may add ARIA role in future).
UPDATE: I see what the problem is all about - Selector specify problem. I think the best fix is changing the selector of height rulesets to e.g.: .bx--list-box.bx--list-box--xl
| .#{$prefix}--dropdown:not(.#{$prefix}--list-box) { | ||
| height: rem(40px); | ||
| } | ||
|
|
||
| .#{$prefix}--dropdown--xl { |
There was a problem hiding this comment.
@asudoh can you explain what these were added for?
|
@asudoh The problem, as you mentioned, is the lack of list-box styles in Vanilla. My first solution was to remove the height from Dropdown (because it's defined by list-box), however this means Vanilla has no height and breaks. More specificity is not the correct way about this. That makes it even more difficult for someone to theme/style this component. We would need to add extra specificity to all the list box components, not just the Dropdown. I updated the solution to use a class rather than role. |
|
I still think the problem is selector specificity, because if you take a look at the Styles tab in Chrome DevTools, we see Vanilla dropdown not having |
|
@asudoh the instructions you gave are the same ones I gave in the issue I opened. The overall issue here is the dropdown styles are competing with the listbox styles. The best solution is to remove the height from Dropdown (my first solution). This doesn't work because Vanilla can't use listbox height. So that gives us these options:
If we use your solution, someone styling our component would need to use two classes just to match the specificity you're proposing. In addition that impacts Multiselect, Combobox AND Dropdown for all frameworks. |
|
I see specificity race is not about dropdown vs. list box. It's rather about default height of the list box vs. specific size variant of list box, and making sure the size variant works regardless of ruleset definition order. BTW |
|
@asudoh correct, it adds specificity to a single framework for a single component. Your solution adds specificity for all list box components in all frameworks. |
|
The important thing is about "regular version". If we add specificity to the regular version, specificity of specific variants will be affected. |
|
@asudoh those variants would be written only for vanilla? It seems that's what you were attempting to do with the |
|
No, those variant are both for vanilla/React. |
|
@asudoh you didn't use those classes in your PR. The prop you implemented specifically impacts ListBox |
|
The problem is that a change to selector order (among components) seems to have happened after the dropdown size variant change is checked in, and thus I didn't notice (potential at then) selector order/specificity problem when I implemented it. |
|
@asudoh this problem existed in the preview for your PR. |
|
Sounds strange, sorry then. UPDATE: I now see the issue with Netlify preview was likely to have been caused by rebasing. The dropdown size variant PR was created 29 days ago, and a change that is likely to have changed the ruleset definition order in question was merged 26 days ago. |
Agree, this must be what happened, I can't imagine everyone would have approved the PR, design included otherwise. |
|
@alisonjoseph the only thing that was broken was dropdown xl. dropdown sm and the other listbox components worked. I can definitely see how it was overlooked. Regardless of how it was caused, I think this is the best way forward. |
|
I suggest making size variants work regardless of selector definition order, instead of patching to |
|
@asudoh my solution eliminates the order problem by deleting the height off of the drop down. It only supply’s the height when it’s not defined by the list box class. |
|
The problem with |
|
@asudoh your solution increases the specificity of not just drop down but all listbox components. 3 components on all frameworks. This rule only applies to drop downs which are not list boxes (vanilla) 1 component 1 framework. |
The increased specificity is how the selectors should have been defined, to make sure we override the default height if it's defined (or application defines the default height). |
|
@asudoh I disagree. We don’t define the height for the other list box component. Increasing their specificity needlessly would be a bad decision. |
|
Important disagreement in what you just said: we should never implement a feature by trying to overcome/exert styles with extra specificity. That results in fragile, difficult to theme styles. |
|
CSS specificity is used to ensure the more specific the selector is, the more the associated CSS rules are prioritized. Therefore, exact use case for size variants. |
|
Alright @asudoh , I look forward to your fix! We’re waiting on the large drop down for the website. |
Testing: test all the possible sizes for dropdown using the storybook knob
Dropdown isn't implemented as a listbox in vanilla but is in React. In React we rely on the list box height for things like size. Vanilla can't rely on listbox height so we define it here.