feat(list-box): introduce size variants #4731
Conversation
|
Deploy preview for the-carbon-components ready! Built with commit 28cf82e https://deploy-preview-4731--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit 28cf82e https://deploy-preview-4731--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements failed. Built with commit 28cf82e https://app.netlify.com/sites/carbon-elements/deploys/5deeb5cb3843a200073a88cd |
| @@ -66,6 +66,36 @@ | |||
| } | |||
| } | |||
|
|
|||
| .#{$prefix}--dropdown--xl { | |||
There was a problem hiding this comment.
Style nit: should not be nesting styles
| max-height: rem(48px); | ||
|
|
||
| .#{$prefix}--dropdown-text { | ||
| padding-top: rem(11px); |
There was a problem hiding this comment.
Is there any way we can center what is rendered instead of specifying exact pixel values and position relative?
| max-height: rem(48px); | ||
|
|
||
| .#{$prefix}--list-box__field { | ||
| height: rem(48px); |
There was a problem hiding this comment.
Would it make sense for the field to be height: 100%?
|
Simplified the styles without the paddings and removed nesting. |
asudoh
dismissed
joshblack’s stale review
Resetting the review status as all comments has been addressed.
| } | ||
|
|
||
| .#{$prefix}--dropdown--sm { | ||
| height: rem(32px); |
There was a problem hiding this comment.
Is it safe to set height and max-height if default text size changes? Should we use min-height?
|
Usage of |
asudoh
dismissed
joshblack’s stale review
Updating the review status as all the review comments have been addressed.
| @@ -86,6 +86,16 @@ $list-box-menu-width: rem(300px); | |||
| } | |||
| } | |||
|
|
|||
| .#{$prefix}--list-box--xl { | |||
| height: rem(48px); | |||
| max-height: rem(48px); | |||
There was a problem hiding this comment.
Seems like max-height is still being used? Context from previous review comments that were said to have been addressed: https://github.com/carbon-design-system/carbon/pull/4731/files#r349730150
There was a problem hiding this comment.
For this part, the list box gets shrunk down if we remove max-height rule here, given .bx--list-box has max-height rule.
asudoh
dismissed
joshblack’s stale review
All open questions have been addressed.
| @@ -66,6 +66,22 @@ | |||
| } | |||
| } | |||
|
|
|||
| .#{$prefix}--dropdown--xl { | |||
There was a problem hiding this comment.
@asudoh Where is these classes used? Why was it added?
There was a problem hiding this comment.
Started with targeted search, but designer told us that such dropdown's size variant should not be just for targeted search but also for dropdown in general. You can try this at Storybook.
There was a problem hiding this comment.
@asudoh dropdown uses the listbox class for size though. This isn't used at all as far as i can tell?
There was a problem hiding this comment.
@asudoh am I missing something or are these classes never used?
There was a problem hiding this comment.
Though there is no demonstration, vanilla users should be able to use the class.
There was a problem hiding this comment.
@asudoh So we added classes that are strictly meant for a specific framework? That seems...not great. At the very least I don't think they should have been lumped in to this PR unused.
There was a problem hiding this comment.
Not for a specific framework, but for a legacy markup. Removing the legacy markup of the dropdown didn't fit in v10 timeframe.
Refs #3513.
Changelog
New
xl/smsize variants for<Dropdown>,<ComboBox>,<MultiSelect>(and its filterable variant).Testing / Reviewing
Testing should make sure above components are not broken.