Skip to content

fix DateInput format calendar bounds #6438

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 8, 2022
Merged

fix DateInput format calendar bounds #6438

merged 4 commits into from
Nov 8, 2022

Conversation

BrunoViveiros
Copy link
Contributor

@BrunoViveiros BrunoViveiros commented Oct 19, 2022 •
edited
Loading

What does this PR do?

Fix the issue #6159

Where should the reviewer start?

DateInput component

What testing has been done on this PR?

  • Test

How should this be manually tested?

add

format="mm/dd/yyyy"

and

calendarProps={{
  bounds: ['2022-10-10', '2022-10-20'],
}}

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?

I'll need #6424 to be merged first, because I'll need the setHoursWithOffset function to solve a issue with dates with wrong offset

What are the relevant issues?

closes #6159

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

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

@jcfilben
Copy link
Collaborator

It looks like this bug may have already been solved in #6214 I am no longer able to reproduce the issue

@BrunoViveiros
Copy link
Contributor Author

With click the bounds work correctly, the problem occurs when you try to write the date in the input (It lets you select any date). I should have explained better

@jcfilben
Copy link
Collaborator

With click the bounds work correctly, the problem occurs when you try to write the date in the input (It lets you select any date). I should have explained better

Ah okay I see thanks for explaining

taysea
taysea reviewed Oct 20, 2022
View reviewed changes
taysea
taysea reviewed Oct 20, 2022
View reviewed changes
taysea
taysea reviewed Oct 20, 2022
View reviewed changes
taysea
taysea reviewed Oct 20, 2022
View reviewed changes
taysea
taysea reviewed Oct 20, 2022
View reviewed changes
taysea
taysea reviewed Oct 20, 2022
View reviewed changes
taysea
taysea reviewed Oct 20, 2022
View reviewed changes
taysea
taysea reviewed Oct 20, 2022
View reviewed changes
Copy link
Collaborator

@taysea taysea 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 on this! The behavior seems to be working as expected. My comments are more related to some Grommet naming conventions/code cleanups.

taysea
taysea reviewed Oct 21, 2022
View reviewed changes
@taysea taysea self-requested a review October 21, 2022 20:46
taysea
taysea reviewed Oct 21, 2022
View reviewed changes
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

This is looking really great. The last thing that would be great to add is a unit test so we can make sure not to have regressions in the future.

This test can mirror the set up you used locally to test the behavior. We could want the test to:

  • Type in an "invalid" date to the DateInput
  • Check that the value is still undefined

Let me know if you need any help writing the test.

@jcfilben jcfilben added the waiting Awaiting response to latest comments label Oct 24, 2022
@BrunoViveiros
Copy link
Contributor Author

@taysea I have created the tests, could you check?

@jcfilben jcfilben removed the waiting Awaiting response to latest comments label Nov 3, 2022
taysea
taysea approved these changes Nov 8, 2022
View reviewed changes
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

Test looks good to me, thank you!

@ericsoderberghp ericsoderberghp merged commit f9c351e into grommet:master Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taysea taysea taysea approved these changes

+1 more reviewer

@ericsoderberghp ericsoderberghp ericsoderberghp approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible Bug: Is possible to select the last month's day outside DateInput calendarProps bounds.
4 participants
@BrunoViveiros @jcfilben @ericsoderberghp @taysea