Skip to content

Conversation

taysea
Copy link
Collaborator

@taysea taysea commented Jun 7, 2023

What does this PR do?

The MaskedInput inside of DateInput is controlled. Therefore, in an uncontrolled form, when the form is reset, the text value in the DateInput isn't. This PR updates DateInput to listen for the reset event to reset the textValue.

Where should the reviewer start?

src/js/components/DateInput/DateInput.js

What testing has been done on this PR?

Tested with code snippet provided in #6818

Also tested with:

import React from 'react';

import { DateInput, Form, TextInput, Button } from 'grommet';

export const Simple = () => (
  <>
    <Form
      onChange={(value) => {
        console.log(JSON.stringify(value));
      }}
      onReset={(e) => {
        console.log('my reset');
        e.stopPropagation();
      }}
    >
      <DateInput format="mm/dd/yyyy" name="date" />
      <TextInput name="text" />
      <Button label="clear" type="reset" />
    </Form>

    <Form
      onChange={(value) => {
        console.log(JSON.stringify(value));
      }}
    >
      <DateInput format="mm/dd/yyyy" name="date-2" />
      <TextInput name="text-2" />
      <Button label="clear" type="reset" />
    </Form>
  </>
);

Simple.parameters = {
  chromatic: { disable: true },
};

export default {
  title: 'Input/DateInput/Simple',
};

Added jest test.

How should this be manually tested?

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?

Closes #6818

Screenshots (if appropriate)

Do the grommet docs need to be updated?

No.

Should this PR be mentioned in the release notes?

Yes. Fixed bug where DateInput text value did not reset with Form reset.

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

Backwards compatible.

@taysea taysea requested a review from jcfilben June 7, 2023 23:04
@jcfilben jcfilben added the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Jun 8, 2023
@taysea taysea marked this pull request as draft June 8, 2023 19:51
@taysea
Copy link
Collaborator Author

taysea commented Jun 8, 2023

Converted to draft -- this is a little too generic where any reset event is clearing the field (should be isolated to the form the input is in)

@taysea
Copy link
Collaborator Author

taysea commented Jun 8, 2023

This is ready for review again, I've added a check to make sure the DateInput is part of the form that was reset.

@taysea taysea marked this pull request as ready for review June 8, 2023 20:25
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! Thanks for adding a test

@jcfilben jcfilben requested a review from MikeKingdom June 13, 2023 19:10
@jcfilben jcfilben removed the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Jun 13, 2023
@jcfilben jcfilben requested a review from halocline June 20, 2023 17:50
Comment on lines 162 to 163
window.addEventListener('reset', handleFormReset);
return () => window.removeEventListener('reset', handleFormReset);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit of a corner case, but because this expects the event to bubble up, if the Form's onReset stops the event propagation this won't get called. For example I broke this with:

<Form onReset={(e) => {
  console.log('MyReset');
  e.stopPropagation();
}}>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm -- that's a good note, even if a corner case. I was trying to just rely on native HTML events, but I suppose one option would be to fire a custom event within form. and listen for that within DateInput?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I adjusted the scope of the event listener to be placed on the form as opposed to the window. I checked with the code you shared (with custom reset function) and it worked.

@taysea taysea requested a review from MikeKingdom June 28, 2023 20:57
Copy link
Collaborator

@MikeKingdom MikeKingdom left a comment

Choose a reason for hiding this comment

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

Coolness. Thanks!

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.

Nice work!

@jcfilben jcfilben merged commit 5a6ec98 into grommet:master Jun 29, 2023
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.

DateInput in uncontrolled form - form reset - ui not reseted
3 participants