Skip to content
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

feat(datetime): formatOptions property for Datetime #29065

Merged
merged 5 commits into from
Feb 22, 2024
Merged

feat(datetime): formatOptions property for Datetime #29065

merged 5 commits into from
Feb 22, 2024

Conversation

mapsandapps
Copy link
Contributor

@mapsandapps mapsandapps commented Feb 15, 2024 •
edited
Loading

Issue number: Internal

What is the current behavior?

The Datetime header, Datetime time button, and Datetime Button have default date formatting that cannot be set by the developer.

What is the new behavior?

  • The developer can customize the date and time formatting for the Datetime header and time button
  • The developer can customize the date and time formatting for the Datetime Button
  • A warning will appear in the console if they try to provide a time zone (the time zone will not get used)
  • A warning will be logged if they do not include the date or time object for formatOptions as needed for the presentation of the Datetime

Does this introduce a breaking change?

  • Yes
  • No

Other information

These changes have been reviewed in #29009 and #29059. This PR just adds them to the feature branch now that the separate tickets are complete.

mapsandapps and others added 2 commits February 15, 2024 09:55
@mapsandapps @Ionitron
feat(datetime): formatOptions for time button and header (#29009)
7ac08bc 
Issue number: Internal

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->
The Datetime header and time button have default date formatting that
cannot be set by the developer.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- The developer can customize the date and time formatting for the
Datetime header and time button
- A warning will appear in the console if they try to provide a time
zone (the time zone will not get used)

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->

<!-- 
## Other information

Any other information that is important to this PR such as screenshots
of how the component looks before and after the change. -->

---------

Co-authored-by: ionitron <[email protected]>
@mapsandapps @liamdebeasi
feat(datetime-button): formatOptions for Datetime Button (#29059)
2927741 
Issue number: Internal

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->
The Datetime Button has default date formatting that cannot be set by
the developer.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- The developer can customize the date and time formatting for the
Datetime Button
- A warning will be logged if they do not include the `date` or `time`
object for formatOptions as needed for the presentation of the Datetime

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->

---------

Co-authored-by: Liam DeBeasi <[email protected]>
@github-actions github-actions bot added package: core @ionic/core package package: angular @ionic/angular package package: vue @ionic/vue package labels Feb 15, 2024
@mapsandapps mapsandapps marked this pull request as ready for review February 15, 2024 22:30
sean-perkins
sean-perkins suggested changes Feb 17, 2024
View reviewed changes
core/src/components/datetime/utils/format.ts Outdated
/**
* We do not want to display the time zone name
*/
delete formatOptions.timeZoneName;
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we should be using delete here.

This will mutate the original reference passed into the function.

For example, check the console logs in this example: https://codepen.io/sean-perkins/pen/BabMMBK.

You will see that after, the object passed into the helper function no longer reflects it's original value.

Instead, we should be creating a new object without the timezone.

return {
  ...formatOptions
  timeZone: 'UTC',
  timeZoneName: undefined
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in d9239e1

sean-perkins
sean-perkins suggested changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Feature works great, nice work!

Just a few clean-up items listed.

I think an example in one of our HTML templates that demonstrates the format options feature could be useful as well (doesn't need to be connected to a playwright test).

core/src/components/datetime/utils/warn.ts Outdated
@@ -0,0 +1,52 @@
import { printIonWarning } from '@utils/logging';
Copy link
Contributor

Choose a reason for hiding this comment

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

~ Optional: I think validate.ts would be a better name for what this file is responsible for and the functions within it.

"Warn" is what is happening as a side-effect of validating the data coming in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 85a7d73

core/src/components/datetime/utils/warn.ts Outdated
formatOptions?.time?.timeZone ||
formatOptions?.time?.timeZoneName
) {
printIonWarning('Datetime: "timeZone" and "timeZoneName" are not supported in "formatOptions".');
Copy link
Contributor

Choose a reason for hiding this comment

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

In most places that we use printIonWarning, we additionally log the element reference that is associated with the warning. This helps developers know which element is throwing the warning, when they have multiple on the view.

I think it'd be worth passing the element reference into these functions (all in this file) and log the element reference as the second argument:

printIonWarning("Your message", el);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 85a7d73

also added the HTML templates in 016b60a

sean-perkins
sean-perkins approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Great work 🎉

@mapsandapps mapsandapps merged commit 7cdbc1b into feature-7.8 Feb 22, 2024
44 checks passed
@mapsandapps mapsandapps deleted the format-options branch February 22, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@averyjohnston averyjohnston averyjohnston approved these changes

@sean-perkins sean-perkins sean-perkins approved these changes

@liamdebeasi liamdebeasi Awaiting requested review from liamdebeasi

@thetaPC thetaPC Awaiting requested review from thetaPC

Assignees
No one assigned
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mapsandapps @sean-perkins @averyjohnston