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(NumberInput): add warn prop #6652

Merged
merged 7 commits into from
Sep 22, 2020

Conversation

janhassel
Copy link
Member

@janhassel janhassel commented Aug 12, 2020

Related: #5918

This PR adds a warn prop to the NumberInput component to follow the behaviour added to the text input so far (see issue PR above).

Changelog

New

  • Added props.warn and props.warnText to NumberInput

Changed

  • Updated css selector in _form.scss to display warning messages (when input has data-warn)

Testing / Reviewing

  • Errors (invalid state) should always overpower warnings
  • Warning message should appear instead of helper text
  • Verify visual style with the already implemented warn prop of TextInput

@janhassel janhassel requested a review from a team as a code owner August 12, 2020 07:50
@netlify
Copy link

netlify bot commented Aug 12, 2020

Deploy preview for carbon-elements ready!

Built with commit a122d41

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

@netlify
Copy link

netlify bot commented Aug 12, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit a122d41

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

@@ -78,10 +78,11 @@

input[data-invalid],
.#{$prefix}--number[data-invalid] .#{$prefix}--number__input-wrapper,
.#{$prefix}--number[data-warn] .#{$prefix}--number__input-wrapper,
Copy link
Contributor

@joshblack joshblack Aug 13, 2020

Choose a reason for hiding this comment

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

Was looking at this PR real quick, is data-warn to match the data-invalid convention above? 👀 If it helps, I think we could go down a class-driven approach for this as the data-* states traditionally come from vanilla if I remember correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this was mainly to follow the same "predictable" behaviour for the developer. In case a custom style or some logic is in place that utilizes [data-invalid] as the css selector, it would be more intuitive to select the warning state with [data-warn] than by naming the classes.

I'm open to changing it to a classname though, in which case I would also update the TextInput.js to remove its data-warn and also soley rely on the --warning modifier.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joshblack Just to clarify now, do we want to follow the [data-warn] or the --warning class convention?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @janhassel! So sorry about the delay, --warning should be good going forward!

Copy link
Member Author

Choose a reason for hiding this comment

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

@joshblack All good. Changed it accordingly 🙂 (cc @emyarod @andreancardona)

@emyarod emyarod requested review from a team, designertyler and jeanservaas and removed request for a team and designertyler September 11, 2020 15:10
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 pending visual review

@joshblack
Copy link
Contributor

bump @andreancardona @jeanservaas when you have a chance to review 👀

Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

@janhassel looks good!

@joshblack
Copy link
Contributor

bump @jeanservaas @aagonzales if you have a sec to review 👀

Copy link
Member

@aagonzales aagonzales 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! Matches the TextInput warning.

@kodiakhq kodiakhq bot merged commit bcc0bc3 into carbon-design-system:master Sep 22, 2020
@janhassel janhassel deleted the number-input-warning branch September 23, 2020 07:35
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.

6 participants