Skip to content

fix(datetime-button): render correct text when passing partial date values#27816

Merged
liamdebeasi merged 8 commits into
mainfrom
FW-4778
Aug 1, 2023
Merged

fix(datetime-button): render correct text when passing partial date values#27816
liamdebeasi merged 8 commits into
mainfrom
FW-4778

Conversation

@liamdebeasi

@liamdebeasi liamdebeasi commented Jul 18, 2023

Copy link
Copy Markdown
Contributor

Issue number: resolves #27797


What is the current behavior?

Datetime Button passes a parsed value to one of the many text formatting utilities we have, such as getMonthAndYear. However, developers can pass partial date values such as 2022 or 2022-04 (April 2022). According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date#date_time_string_format, these are still valid date strings.

However, the parseDate utility does not add fallback values. So passing 2022 will cause the day and month fields to be undefined. This means that getNormalizedDate passes '//2022' to the Date constructor.

Some browsers, such as Chrome, will automatically account for the stray slashes and still return a valid date. Other browsers, such as Safari, do not do this and will either return "Invalid Date" or throw an error.

What is the new behavior?

  • Date normalizing utility now has fallback values so we always pass in a valid date. In the example above, getNormalizedDate will now pass '1/1/2022' instead of '//2022' to the Date constructor.
  • Refactored other utils that use new Date to make use of getNormalizedDate since they are also impacted.

Note: I added an E2E test instead of a spec test because I want to test cross-browser behavior to ensure consistency.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@bolt-new-by-stackblitz

Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions Bot added the package: core @ionic/core package label Jul 18, 2023
@liamdebeasi
liamdebeasi marked this pull request as ready for review July 18, 2023 15:21
* account for this and still return a valid date. However,
* this is not a consistent behavior across all browsers.
*/
return new Date(`${refParts.month ?? 1}/${refParts.day ?? 1}/${refParts.year ?? 2023}${timeString} GMT+0000`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we fallback to the current year, instead of hardcoding 2023?

e.g.:

refParts.year ?? new Date().getFullYear()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could, though we also default to 1/1/2023 here:

Any preference?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't have a preference myself. Presumably, if I'm passing in a value without the year, I'm using the month presentation, in which case I don't care about the year since it'll never be displayed.

That said, if we do go with the current year, I would rather use the current date for the month and day as well for consistency's sake.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Presumably, if I'm passing in a value without the year, I'm using the month presentation, in which case I don't care about the year since it'll never be displayed.

Yep exactly this. If any of those values are undefined then they aren't actually used in the rendering of the component -- we just need them to get a valid date

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm ok with this as-is then 👍

@liamdebeasi
liamdebeasi requested a review from sean-perkins July 25, 2023 17:07
@liamdebeasi
liamdebeasi added this pull request to the merge queue Aug 1, 2023
Merged via the queue into main with commit bd1910b Aug 1, 2023
@liamdebeasi
liamdebeasi deleted the FW-4778 branch August 1, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: date value is not finite in DateTimeFormat format() when presentation='year' on ios device

3 participants