fix(list-box): ensure size variants are in effect

[asudoh]

This change ensures the style rule in .bx--list-box--xl for dropdown size wins over one in .bx--dropdown or any ones in application styles (for default size), by expressing that they are for non-default styles and should win order the one for the default size.

Fixes #4916.

Changelog

Changed

• Adding .bx--list-box to the selector of size variant style rule, so it wins over the style rule for the default height.

Testing / Reviewing

Testing should make sure combo box, dropdown, multi select are not broken.

[netlify]

Deploy preview for the-carbon-components ready!

Built with commit fdf545d

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

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit fdf545d

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

[netlify]

Deploy preview for carbon-elements failed.

Built with commit fdf545d

https://app.netlify.com/sites/carbon-elements/deploys/5e1e664d1bcd2e00087e8145

[vpicone]

@asudoh I don't believe this is the correct approach. We don't want double the specificity for all listbox components it makes it very difficult to override and theme. Say for instance someone wanted to make the 'large' button larger in their theme. This would be unexpected specificity and require important or double/triple classNames.

This is also different than how we do variants in the codebase. Take Button for example: the variants (size and kind) don't use double class names to style.

[vpicone]

Also wondering if the dropdown options should be the same size as the field. @carbon-design-system/design

[mjabbink]

yes. The dropdown and fields below should be same. so for 32px dd then 32px for content, and for 40 and 48px options the same I think. @jeanservaas?

[asudoh]

Wrt visual design it makes sense. Given it does not seem to be in the scope of #4916, created #4951 to track that.

Wrt the CSS rule, I see the point of difference from buttons, while I think it's a sensible difference given where we are at. Just in case - Would you want to suggest an alternate approach @vpicone?

[vpicone]

@asudoh yes, creating an exception for non-listbox implementations (vanilla) as I did in my original PR would both prevent excess specificity and allow us to use the strategy found in other variant implementations (such as button).

[asudoh]

Refs: #4917 (comment)

[vpicone]

@asudoh removing the height from dropdown and instead using the listbox styles solves the problem.

We need an exception for vanilla which does not use listbox:

  .#{$prefix}--dropdown:not(.#{$prefix}--list-box) {
    height: rem(40px);
  }

This approach means only vanilla (and even then only the dropdown) is impacted by extra specificity.

I believe this is a better approach compared to doubling the specificity for all frameworks and for all three list box components. At this point, we shouldn't be sacrificing style quality for Vanilla. Since this is a new feature, we could have made the Vanilla implementation require listbox classes to function (or even better, not introduce these variants for vanilla at all)

[asudoh]

Just in case it clarifies, the list box classes cannot be applies to the legacy markup due to the nature of difference in markup.

[emyarod]

it looks like .bx--dropdown--xl is already in the stylesheet, it is just not being added to the React component. since multiselect and combobox are not present in the vanilla library and dropdown does not use listbox classes in vanilla, should we just apply the .bx--dropdown--xl class to the React component for now?

[asudoh]

@emyarod Sounds 💯, now I wonder why I didn't come up with such idea... Updated.

Got a different technical approach.

[emyarod]

was the selector change intentional?

[asudoh]

Oops good catch @emyarod! Not sure why the wrong selectors got into the commit...

[emyarod]

thanks for making the change, looks good to me

[vpicone]

@joshblack @emyarod those drop down selectors were added with @asudoh’s original PR last month. They went unused in that PR (#4731)

The size functionality in react got randomly lumped in with this unrelated PR: #4999. Now this PR just wires it up?

Isn’t weird the other listbox components don’t need their own size variant classes? Or that we're still passing the size into the Listbox of dropdown?

Also the arrow is misaligned for the size variants in iOS:

[joshblack]

@vpicone oh wow, did not realize this. How can we address this? Do we need to revert this PR?

[vpicone]

@joshblack this was my recommendation and has context/explanation in the comments #4917

The reality is our vanilla implementation of dropdown differs from react (it doesn’t use listbox). So we have to choose the “least wrong” way to style these variants. That PR has a bunch of info on why I thought that approach was the lesser of two evils. Optimizing for parity with other listbox components and minimizing specificity.

The issue with reverting this PR is that the actual substance of the implementation (wiring the selectors to the react component) is in an unrelated PR: #4999 . That PR likely has changes we don’t want to revert.

[asudoh]

.bx--dropdown--xl is actually something for use - It was introduced so that size variants can be used in legacy markup.

[vpicone]

@asudoh So we're okay with including classes/styles for particular frameworks now?

You've now made it the implementation for React as well as legacy markup. Now React Dropdown get's size variant from both Listbox and this new Dropdown class you've added.

[asudoh]

The legacy markup is not for particular frameworks, just a left-over from earlier release. That said, usage of .bx--dropdown--xl class is an interim solution until we can make a breaking change.

[vpicone]

@asudoh legacy markup is vanilla no? The Dropdown is now styled by both these new classes you added for the “legacy system” and listbox classes.

[asudoh]

As stated earlier, cleaning up legacy code can happen only when we can make a breaking change.

[vpicone]

@asudoh but this isn’t legacy it’s a new feature. You just added these selectors: asudoh@0723836#diff-d69d1c6c9ae9007ad44b45f8f333fa3f

Were they only meant for vanilla?

[asudoh]

New feature, while given it's in v10 codebase, compatibility with existing codebase in v10 needs to be taken into account.