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.

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(number-input): update number input controls to new spec #7687

Merged

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Jan 28, 2021

Closes #5417

unblocks #7570

This PR updates the number input step controls and invalid/warning states to match the updated component spec. This PR is only focused on the input controls and the remaining ticket issues will be addressed in separate PRs related to #7570

Testing / Reviewing

Confirm all size variants and warning states for the number input match the new spec

@netlify
Copy link

netlify bot commented Jan 28, 2021

Deploy preview for carbon-elements ready!

Built with commit ce0af49

https://deploy-preview-7687--carbon-elements.netlify.app

@tw15egan
Copy link
Member

tw15egan commented Jan 28, 2021

More of a design question, but should we set .bx--number to width: 100%; to conform with the changes we've made to Select and Dropdown and the existing TextInput? Or let the end-user determine how large it should be?

Currently, it is getting cut off after 3 digits when it exceeds the max
Screen Shot 2021-01-28 at 12 40 58 PM

Edit:

Actually, nevermind. It seems like it adjusts the width based on the max value of the NumberInput. If I set it to
Screen Shot 2021-01-28 at 12 45 58 PM

I get

Screen Shot 2021-01-28 at 12 46 07 PM

So I think this actually is working great. Disregard! 👍

@emyarod
Copy link
Member Author

emyarod commented Jan 28, 2021

yeah this is caused by an arbitrary max-width we placed on the number inputs

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@netlify
Copy link

netlify bot commented Jan 28, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit ce0af49

https://deploy-preview-7687--carbon-components-react.netlify.app

@emyarod emyarod force-pushed the 5417-number-input-control-update branch 2 times, most recently from c01daaf to 9cafbcb Compare January 28, 2021 21:11
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.

Disabled state

  • The - and + icons should not get the bottom rule.

Screen Shot 2021-02-01 at 11 36 11 AM

Light variant

  • There are small strips of color around the - and + icons that should not be showing up in the input.

Screen Shot 2021-02-01 at 11 52 03 AM

Invalid state

  • The - and + icons seem to shift down a pixel when the invalid state is applied. This happens with all sizes.
    Feb-01-2021 11-37-24

Is it possible to hide the divider rule between the warning state/invalid state icon when hovering on the - icon?
Screen Shot 2021-02-01 at 11 50 23 AM

@emyarod emyarod force-pushed the 5417-number-input-control-update branch from 9cafbcb to 602e9c8 Compare February 3, 2021 16:55
@emyarod emyarod force-pushed the 5417-number-input-control-update branch from 565588e to b491a0c Compare February 3, 2021 17:05
@emyarod
Copy link
Member Author

emyarod commented Feb 3, 2021

the icons shifting seems to be a Firefox only issue but it should be resolved now

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.

Hover:

  • The hover colors on the - and + need to be hover-light-ui for the light prop.

Divider rules:

  • The divider rules needs to be decorative-01 in the light prop.
  • The divider rules needs to be disabled-02 for the disabled state. Looks a little lighter right now in all themes.

Invalid state:

  • When hovering over the - and +, there are little dot glitches at the bottom of the input.
  • When hovering over the +, the divider rule between the - and the error/warning icon should not go away.

Feb-03-2021 15-21-17

@emyarod emyarod force-pushed the 5417-number-input-control-update branch 2 times, most recently from fe961bd to a4a2ea3 Compare February 5, 2021 19:27
@emyarod
Copy link
Member Author

emyarod commented Feb 5, 2021

when should the dividers be hidden/not hidden?

@laurenmrice
Copy link
Member

@emyarod

Artboard

@emyarod emyarod force-pushed the 5417-number-input-control-update branch from a4a2ea3 to 9bef35b Compare February 5, 2021 21:26
@emyarod
Copy link
Member Author

emyarod commented Feb 5, 2021

got it, the divider rule styles should be updated now

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.

There are still some issue with the divider rules:

  • Enabled state: the divider rule needs to be present between the - and +. Right now it is not there. Should be:
    enabled

  • Disabled state: the divider rule isn’t showing up in the disabled state. Needs to be disabled-02. Should be:
    disabled

  • Invalid/Warning states: need to have two divider rules by default to separate out the icons. Needs to also follow the hovering logic mentioned in my last comment. Should be:
    invalid

@emyarod emyarod force-pushed the 5417-number-input-control-update branch from 66dcebb to e497cfa Compare February 23, 2021 17:25
@emyarod emyarod force-pushed the 5417-number-input-control-update branch from 3424998 to 656951a Compare February 23, 2021 17:41
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.

Looks great! Thanks for updating this component! 🎉

@kodiakhq kodiakhq bot merged commit 0cfcf7c into carbon-design-system:master Feb 24, 2021
@emyarod emyarod deleted the 5417-number-input-control-update branch February 26, 2021 19:26
@dakahn dakahn mentioned this pull request Mar 4, 2021
@emyarod emyarod mentioned this pull request Mar 8, 2021
60 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Number input] control update
5 participants