Skip to content

feat: set new hours considering the offset #6424

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
Oct 19, 2022
Merged

feat: set new hours considering the offset #6424

merged 4 commits into from
Oct 19, 2022

Conversation

matheushahnn
Copy link
Contributor

@matheushahnn matheushahnn commented Oct 14, 2022 •
edited by halocline
Loading

What does this PR do?

It fixes the issue related to timezone on Calendar

Closes #6421

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Test this storybook https://storybook.grommet.io/?path=/story/input-dateinput-format-inline--format-inline

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?

#6421

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?

@matheushahnn
Copy link
Contributor Author

matheushahnn commented Oct 14, 2022 •
edited
Loading

I still need to change on grommet/src/js/components/Calendar/utils.js
In fact, I am not sure how it should do in this file, because it is adding days.
Do you know where it is failing this utils and how I can reproduce locally?

@halocline
Copy link
Collaborator

halocline commented Oct 14, 2022 •
edited
Loading

@matheushahnn First, thank you for picking this issue up.

I still need to change on grommet/src/js/components/Calendar/utils.js In fact, I am not sure how it should do in this file, because it is adding days. Do you know where it is failing this utils and how I can reproduce locally?

This is actually performing as expected. I flagged this as an opportunity to consolidate logic as it looked like we are doing something somewhat related to the timezone offset work, but in a slightly different manner and didn't know if would be beneficial to bring consistency here. I did not study closely.

If your judgement and recommendation is that this should remain as is, please let us know.

The rest of the work is looking great, thank you.

@matheushahnn
Copy link
Contributor Author

I think we can keep it as it is.
Please, let me know what else I should do in this PR.

jcfilben
jcfilben reviewed Oct 18, 2022
View reviewed changes
@BrunoViveiros BrunoViveiros mentioned this pull request Oct 19, 2022
Merged
5 tasks
jcfilben
jcfilben approved these changes Oct 19, 2022
View reviewed changes
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!

@ericsoderberghp ericsoderberghp merged commit 9a1d9a6 into grommet:master Oct 19, 2022
This was referenced Nov 14, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

DateInput - incorrect timezone offset being applied
4 participants
@matheushahnn @halocline @ericsoderberghp @jcfilben