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

Fix 'undefined' value in MaskedInput #6900

Merged
merged 2 commits into from Jul 27, 2023
Merged

Fix 'undefined' value in MaskedInput #6900

merged 2 commits into from Jul 27, 2023

Conversation

MikeKingdom
Copy link
Collaborator

What does this PR do?

Fixes an issue where the MaskedInput value shows as undefined when the value property isn't supplied and an non-matching character is input.

Fixes #6884

Where should the reviewer start?

MaskedInput.js

What testing has been done on this PR?

Manual, storybook and jest

How should this be manually tested?

export const Barcode = () => {
  const mask = [
    {
      length: [1, 14],
      regexp: /^(3|38|38[0,1]|38[0,1][0-9]{1,11})$/,
    },
  ];

  return (
    // Uncomment <Grommet> lines when using outside of storybook
    // <Grommet theme={...}>
    <Box fill align="center" justify="start" pad="large">
      <Box width="medium">
        <MaskedInput
          mask={mask} 

       />
      </Box>
    </Box>
    // </Grommet>
  );
};

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

#6884

Screenshots (if appropriate)

Do the grommet docs need to be updated?

No

Should this PR be mentioned in the release notes?

Yes

Is this change backwards compatible or is it a breaking change?

Backwards compatible

@@ -185,6 +185,7 @@ const MaskedInput = forwardRef(
const [value, setValue] = formContext.useFormInput({
name,
value: valueProp,
initialValue: '',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we take defaultValue into account here at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we take defaultValue into account here at all?

What defaultValue? MaskedInput doesn't have a defaultValue prop. I think at best the caller would pass that in as a value

Copy link
Collaborator

Choose a reason for hiding this comment

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

TextInput supports a defaultValue -- I'm assuming the "rest" from MaskedInput ends up on the TextInput it's built with. https://github.com/grommet/grommet/blob/master/src/js/components/TextInput/TextInput.js#L477

In some cases like in CheckBoxGroup we do something like initialValue: defaultValue || [] so wasn't sure if that was important to consider here or not. https://github.com/grommet/grommet/blob/master/src/js/components/CheckBoxGroup/CheckBoxGroup.js#L45

We don't have defaultValue documented on MaskedInput at the moment so I would be fine leaving it separate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TextInput supports a defaultValue -- I'm assuming the "rest" from MaskedInput ends up on the TextInput it's built with. https://github.com/grommet/grommet/blob/master/src/js/components/TextInput/TextInput.js#L477

Interesting. Note that it's not documented for TextInput either (https://v2.grommet.io/textinput) and it's not in TextInput's propTypes or typescript from what I can tell. I could add it to the formInput initialValue though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TextInput supports a defaultValue -- I'm assuming the "rest" from MaskedInput ends up on the TextInput it's built with. https://github.com/grommet/grommet/blob/master/src/js/components/TextInput/TextInput.js#L477

And actually it doesn't look like MaskedInput uses TextInput. It passes rest along to StyledMaskedInput which is just a styled.input

So I think we're good here (although poor TextInput probably should have defaultValue documented.)

Copy link
Collaborator

@jcfilben jcfilben 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!

@jcfilben jcfilben requested a review from taysea July 25, 2023 16:09
@jcfilben jcfilben merged commit fe41a9a into master Jul 27, 2023
14 checks passed
@jcfilben jcfilben deleted the maskedinput-undefined branch December 13, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MaskedInput displays "undefined" when unsupported key is entered
3 participants