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.

Sign up for GitHub

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

Jump to bottom

feat(number-input): add size variants #5459

Merged
merged 9 commits into from
Mar 3, 2020
Merged

feat(number-input): add size variants #5459

merged 9 commits into from
Mar 3, 2020

Conversation

tw15egan
Copy link
Collaborator

Closes #5441

Adds in sm and xl versions of NumberInput.

Changelog

New

  • xl and sm styles for NumberInput
  • Knob to change sizes in the storybook

Testing / Reviewing

Go to NumberInput --> knobs and change the size. Ensure the arrows focus border is correct, as well as mobile variant button sizing.

@tw15egan tw15egan requested a review from a team as a code owner February 27, 2020 10:54
@ghost ghost requested review from emyarod and joshblack February 27, 2020 10:54
@netlify
Copy link

netlify bot commented Feb 27, 2020

Deploy preview for carbon-elements ready!

Built with commit 164387d

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

@netlify
Copy link

netlify bot commented Feb 27, 2020 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit ef5a5c0

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

@tw15egan tw15egan mentioned this pull request Feb 27, 2020
Closed
@netlify
Copy link

netlify bot commented Feb 27, 2020

Deploy preview for carbon-elements ready!

Built with commit cd21a5d

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

@netlify
Copy link

netlify bot commented Feb 27, 2020 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit ef5a5c0

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

laurenmrice
laurenmrice requested changes Feb 27, 2020
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

size looks good to me.

We just need to remove the underline for the disabled state:
Screen Shot 2020-02-27 at 11 27 15 AM

@aagonzales aagonzales mentioned this pull request Feb 27, 2020
Closed
24 tasks
jendowns
jendowns reviewed Feb 27, 2020
View reviewed changes
Copy link
Contributor

@jendowns jendowns left a comment

Choose a reason for hiding this comment

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

Hey @tw15egan, just a suggestion to allow an empty string for the size prop. This is related to #5469 & #5471. Thank you! 🎉

packages/react/src/components/NumberInput/NumberInput.js Outdated Show resolved Hide resolved
@tw15egan
Copy link
Collaborator Author

@emyarod @laurenmrice changes made 👍

laurenmrice
laurenmrice approved these changes Feb 28, 2020
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

thanks tj ! looks good to me 👍🏻

emyarod
emyarod approved these changes Feb 28, 2020
View reviewed changes
Copy link
Member

@emyarod emyarod left a 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, just one note

packages/react/src/components/NumberInput/NumberInput-story.js
@@ -28,6 +35,7 @@ const props = () => ({
max: number('Maximum value (max)', 100),
value: number('Value (value)', 50),
step: number('Step of up/down arrow (step)', 10),
size: select('Field size (size)', sizes, undefined) || undefined,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
size: select('Field size (size)', sizes, undefined) || undefined,
size: select('Field size (size)', sizes, undefined),

do we still need this part?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure, we can make another PR to remove them from each of the size examples though if it is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like this is still needed. If you swap to a different size and then swap back to default, I'm seeing a console warning.

abbeyhrt
abbeyhrt approved these changes Mar 2, 2020
View reviewed changes
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 good to me!

@tw15egan tw15egan merged commit d68e6d2 into carbon-design-system:master Mar 3, 2020
@joshblack joshblack mentioned this pull request Mar 31, 2020
Closed
@tw15egan tw15egan deleted the number-input-sizing branch April 28, 2021 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jendowns jendowns jendowns left review comments

@laurenmrice laurenmrice laurenmrice approved these changes

@abbeyhrt abbeyhrt abbeyhrt approved these changes

@emyarod emyarod emyarod approved these changes

@joshblack joshblack Awaiting requested review from joshblack

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[number input] add 48px and 32px size options
5 participants
@tw15egan @emyarod @jendowns @abbeyhrt @laurenmrice