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.

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

feat(datetime): add header text to multiple selection; improve header consistency between modes #25817

Merged
merged 18 commits into from Aug 29, 2022

Conversation

amandaejohnston
Copy link
Contributor

@amandaejohnston amandaejohnston commented Aug 24, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

  • When multiple="true" and the header is enabled (e.g. when showDefaultTitle="true"), the part of the datetime's header that shows what date you have selected (hereafter referred to as just "the header") is not rendered.
  • The header doesn't render in ios mode at all, whether multiple is enabled or not.

Issue URL: resolves #25668

md mode ios mode
md mode ios mode

What is the new behavior?

  • The header now shows if multiple="true". If 0 or more than 1 dates are selected, the header shows the number, rather than the exact selected dates. (The behavior when exactly 1 date is selected is unchanged.)
  • Added a new prop titleSelectedDatesFormatter which can be used to customize the display of the header for localization purposes.
    • Note that this function is not called when exactly 1 date is selected, since that part is already localized automatically. Otherwise, devs would need to replicate the existing localization in their callback to handle this case.
  • The selected date(s) will also now show in the header in ios mode. The text is identical between the two modes in all cases.
    • This also applies to single-selection datetimes.
md mode ios mode
md mode ios mode

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package labels Aug 24, 2022
Comment on lines +24 to 33
font-size: 14px;
}

:host .datetime-header .datetime-title {
color: var(--title-color);
}

font-size: 14px;
:host .datetime-header .datetime-selected-date {
@include margin(10px, null, null, null);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This styling was fairly arbitrary on my part, since we don't have anything existing to go on as far as native comparisons go. I just re-used the same font size and picked a margin that looked good with the existing design.

@amandaejohnston amandaejohnston marked this pull request as ready for review August 24, 2022 21:04
@amandaejohnston amandaejohnston requested a review from a team as a code owner August 24, 2022 21:04
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.

Since this feature changes the behavior of single selection datetime, should we also have similar checks as the multiple tests, to verify that text is rendering correctly (outside of screenshot tests)?

core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved
core/src/components/datetime/datetime-interface.ts Outdated Show resolved Hide resolved
core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved
core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved
core/src/components/datetime/datetime.tsx Show resolved Hide resolved
@amandaejohnston
Copy link
Contributor Author

Since this feature changes the behavior of single selection datetime, should we also have similar checks as the multiple tests, to verify that text is rendering correctly (outside of screenshot tests)?

I would say the screenshot tests cover that, since the header text can only show the one selected date -- unlike when multiple="true", where there are a few different cases to account for. I could be convinced otherwise though.

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 on this! I'm good with or without the rename of the prop.

Copy link
Member

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Is integrating datetime-button with these changes part of this work, or would you recommend we do separate design work for that?

core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved
@amandaejohnston
Copy link
Contributor Author

I figure updating datetime-button is a separate task.

Copy link
Member

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Great job! The property name is a bit wordy, but I also can't think of a shorter name that is still descriptive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants