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 parts for calendar day, active, and today #27641

Merged
merged 33 commits into from
Sep 6, 2023

Conversation

brandyscarney
Copy link
Member

@brandyscarney brandyscarney commented Jun 13, 2023

Issue number: resolves #25340


What is the current behavior?

It is not possible to style the calendar days in a datetime since it uses shadow DOM and there are not exposed parts or CSS variables for the calendar days.

What is the new behavior?

  • Exposes the following parts for a calendar day: calendar-day, today, and active
  • Combines the calendar-day-highlight element with the calendar-day element so developers don't have to know to style two different elements & we don't have to expose them as separate parts
  • Improves height parity of the calendar day across browsers
  • Updates the custom e2e test to include an example of styling days using the newly exposed CSS parts
  • Adds tests for the focus states of the calendar day

Does this introduce a breaking change?

  • Yes
  • No

Other information

This PR has a number of screenshot diffs. I have gone through all of them and believe they are either improvements to datetime or inconsequential changes.

  1. Material Design height changes - several screenshots show the height of the datetime for md increasing, this comment explains why this is a good change. (I did not comment on every screenshot with a height increase, but you can see the pixel change at the bottom to verify it was caused by this).
  2. iOS height changes - several screenshots show a shift in the height of the datetime for ios. This is caused by the browsers rendering fractional pixels differently. This comment explains why this is a desired change. (I did not comment on every screenshot with a height increase, but you can see the pixel change at the bottom to verify it was caused by this).
  3. Material Design circle height increase - The calendar day highlight has increased in height for non-active days. I believe this is a good change because I don't see why they would be smaller, but I am open to discussion if there are some guidelines I missed here. This comment shows the difference in height.
  4. Numbers shifting - many, many screenshots show a horizontal shift in 1-3 of the numbers. I added comments to (I believe) all of the screenshots where this is happening because it is quite difficult to see the difference on some. This comment shows a video of the shift.

@stackblitz
Copy link

stackblitz bot commented Jun 13, 2023

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 Jun 13, 2023
@brandyscarney brandyscarney marked this pull request as ready for review June 14, 2023 20:55
@brandyscarney brandyscarney marked this pull request as draft June 20, 2023 19:27
Base automatically changed from feature-7.1 to main June 21, 2023 14:03
@liamdebeasi
Copy link
Contributor

Going to remove myself from review while this is in draft. Feel free to re-add once it's ready for review!

@liamdebeasi liamdebeasi removed their request for review August 21, 2023 15:02
Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 12 and the 26.

datetime-diff-one.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 12 and the 26.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 12 and the 26.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 12 and the 26.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 4.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 1 in 14 and 0 in 30.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 1 and 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 1 and 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 1 and 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between these two screenshots is a shift horizontally of the 1 in 14.

@brandyscarney brandyscarney marked this pull request as ready for review August 22, 2023 17:16
core/api.txt Show resolved Hide resolved
core/src/components/datetime/test/color/datetime.e2e.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with making them all the same size; not sure why that border would have been there in the first place 🤔

@brandyscarney
Copy link
Member Author

This should be good for review now! All feedback has been addressed & screenshots are up to date.

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.

My comments are non-blocking. Fantastic work on this ✨

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

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

Good to go once below feedback is addressed. Great work!

core/src/components/datetime/datetime.ios.scss Outdated Show resolved Hide resolved
core/api.txt Show resolved Hide resolved
Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

Copy link
Contributor

@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 work!

@mapsandapps mapsandapps removed their request for review September 6, 2023 14:43
@brandyscarney brandyscarney merged commit 79b005d into feature-7.4 Sep 6, 2023
@brandyscarney brandyscarney deleted the FW-1593 branch September 6, 2023 15:58
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.

feat: datetime, expose calendar grid buttons for styling
5 participants