-
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
feat(select): add height variations to select #5311
feat(select): add height variations to select #5311
Conversation
Deploy preview for carbon-elements ready! Built with commit e8bee5d |
Deploy preview for carbon-components-react ready! Built with commit e8bee5d https://deploy-preview-5311--carbon-components-react.netlify.com |
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.
LGTM 👍 - Thanks @tw15egan!
@tw15egan are we missing a step? Was surprised it went from sm -> lg -> xlg without a md bump 🤔 Code looks great otherwise! Just was curious about that bit |
I agree, which is why when I made this PR (#5076) I changed the prop names, but it caused a mismatch with the existing dropdown props, which caused issues. See convo here https://ibm-studios.slack.com/archives/GCQ5R263T/p1579819071014900 This should be changed in v11, and it should be the default |
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 good to me pending visual review
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.
LGTM!
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.
LGTM
@tw15egan was the conflict with dropdown in a release yet? Or was it earlier work? If not in a release I bet we could update it before 10.10. Separately thanks for the links, reading up now 👀 |
Pretty sure Dropdown additions were released in early December, which is why we reverted my changes to be consistent with Dropdown. |
@tw15egan ah got it, you're totally right. Would it make sense if we updated Dropdown to also accept the intended value and use that in docs? Just was trying to understand the ideal here and if we can coordinate the same sizes cross components or not |
@joshblack do we want to merge this in? Or did we want to make the modifications to text input/dropdown to take in |
@tw15egan I think those modifications would definitely be worth it, what do you think? Also doesn't need to block this for sure, could always move over to a separate ticket and we can tackle it before releasing if we think it's worth it. |
@joshblack I think they would be too, let's do that modification in another PR 👍 |
Sounds good! Do we want to use that one button issue or should I go make another one? |
@joshblack we can use the button one 👍 |
Closes #5229
Select now mimics
Dropdown
andTextInput
, and has 3 sizes available.Changelog
New
size
prop toSelect
(acceptssm
andxl
, defaults to standard size)Testing / Reviewing
Should ensure
Select
renders at 3 different heights based on the size prop