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(design-system): add NumberInput component and stories #2267

Merged
merged 9 commits into from
Jul 27, 2021

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Jun 28, 2021

This PR builds on the input component PR #2246

Problem

This PR adds the NumberInput component, as well as stories depicting its various states and how to use it with react-hook-form.

Closes #2004

Solution

Features:

  • feat(theme): add NumberInput theme that extends from Input theme
  • feat: add NumberInput component
  • feat: add NumberInput stories

Before & After Screenshots

In generated storybook.

@karrui karrui added the react label Jun 28, 2021
@karrui karrui requested review from yong-jie, mantariksh and tshuli June 28, 2021 08:33
@karrui karrui force-pushed the form-v2/number-input-component branch from 62ef520 to caaff52 Compare June 30, 2021 08:30
Base automatically changed from form-v2/input-component to form-v2/develop July 7, 2021 07:46
@karrui karrui force-pushed the form-v2/number-input-component branch from caaff52 to e8c6dc9 Compare July 7, 2021 07:47
@seaerchin
Copy link
Contributor

i saw that there was a lock on disabled but it's not present in the storybook - should we add it in?

@karrui
Copy link
Contributor Author

karrui commented Jul 12, 2021

Design changes, moving to draft again

@karrui karrui marked this pull request as draft July 12, 2021 04:45
@karrui karrui requested a review from mantariksh July 12, 2021 10:06
@karrui karrui marked this pull request as ready for review July 12, 2021 10:06
@karrui karrui requested a review from seaerchin July 12, 2021 10:06

return (
<Box {...htmlProps} ref={ref} __css={styles.root}>
<chakra.input
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we using chakra's internal input here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the Input component performs more transformation of inputProps, removing some important variables such as aria-invalid, etc. Resulting in some styles not being provided. I'll add a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible to patch upstream, i will open an issue if I have time....

Comment on lines +50 to +56
const controlProps = useFormControlProps(props)
const {
htmlProps,
getInputProps,
getIncrementButtonProps,
getDecrementButtonProps,
} = useNumberInput({ ...controlProps, clampValueOnBlur })
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it EXTREMELY frustrating that chakra doesn't have any documentation on this. how do you know to use useFormControlProps? how dyou know what args it takes, and that the return value can be passed back into useNumberInput?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at the source code for their own NumberInput implementation.

Reason for using useFormControlProps is so that this component can be wrapped with a FormControl component and be able to access their relevant props, such as whether the field is disabled, readOnly, invalid, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible to patch upstream, i will open an issue if I have time....

@karrui karrui requested a review from mantariksh July 26, 2021 11:15
@karrui karrui merged commit 84fc404 into form-v2/develop Jul 27, 2021
@karrui karrui deleted the form-v2/number-input-component branch July 27, 2021 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants