Improve logic for when to normalize date in DateInput #5942
Conversation
There was a problem hiding this comment.
I think it looks good. I'm trying to think of how to simplify this in general but it's hard to. I'd rather see the normalize flag not go so deep through so many calls and isolate it more to the initial look at defaultValue and value.
Really it feels like the crux of the problem is caring about timezone at all when all we're trying to do is choose a day/month/year. That seems to boil down to how we accept an iso8601 string with all that info in it and try to maintain that format through the api even though we don't really use the time aspect or intentionally convert between timezones. Changing that would probably be breaking.
| } else if (date && Array.isArray(date)) { | ||
| setDate(nextDates); | ||
| } | ||
| setActive(new Date(selectedDate)); | ||
| if (onSelect) { | ||
| // output date with no timestamp if that's how user provided it |
There was a problem hiding this comment.
Can we move all of this conversion logic into a separate utility function? I think that would make this callback code a bit easier to follow.
There was a problem hiding this comment.
I separated this out into its own function. Additionally, I was able to remove blocks of this logic that were never being entered (confirmed via manual testing and coverage analysis in jest).
Yes, you are exactly correct. |
What does this PR do?
When DateInput value starts as undefined, the
onChangelogic should be in UTC relative to the local timezone of the user. In these instances, we don't want to perform the subsequent normalization logic. This PR introduces a state variable in dateinput that is passed ot the normalization functions. When it is true, we proceed with normalization. When it is false, we don't normalize because the date is already in UTC with respect to the timezone.Where should the reviewer start?
DateInput.js
What testing has been done on this PR?
Format Inline story in timezones before UTC and after (for example, pacific time and eastern european.
How should this be manually tested?
With FormatInline story in various timezones.
Do Jest tests follow these best practices?
N/A did not touch tests aside from fixing a test that did not have
defaultValuein ISO8601 format.screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Closes #5920
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.