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

fix(date-picker): end-range is now rounded and has the correct box-shadow #6216

Merged
merged 7 commits into from
Jan 20, 2023

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Jan 3, 2023

Related Issue: #5544

Summary

Improvements to date-picker-day styling include:

  • end-range is now rounded
  • has the correct box-shadow
  • styling is consistent for both dir="ltr" and dir="rtl"

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jan 3, 2023
@Elijbet Elijbet marked this pull request as ready for review January 3, 2023 01:22
@Elijbet Elijbet requested a review from a team as a code owner January 3, 2023 01:22
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 3, 2023
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 3, 2023
@Elijbet Elijbet marked this pull request as draft January 3, 2023 02:32
Copy link
Contributor

@anveshmekala anveshmekala left a comment

Choose a reason for hiding this comment

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

LGTM!

@eriklharper
Copy link
Contributor

End selected highlight circle looks good! One thing I'm noticing that we should also do is make sure the selected end date's hover styles match that of the start date. On your branch looks like there is still a visible gray square that appears behind the blue circle on just the end date of the range. We should match that to look just like the start date, where there is no gray square behind it:

image

@@ -221,7 +221,7 @@
}

:host([highlighted]:not([active]:focus)) .day {
@apply text-color-1 rounded-none;
@apply text-color-1 rounded-full;
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to add rounded-full here as it is the default style applied on .day

@Esri Esri deleted a comment from anveshmekala Jan 3, 2023
@Elijbet
Copy link
Contributor Author

Elijbet commented Jan 8, 2023

I have a couple of design questions on date-picker (mostly hover behavior). @SkyeSeitz @ashetland @macandcheese

  • Should there be a hover style for unselected dates outside of the range, such as an empty bordered circle?
  • Should this also match the hover style of unselected dates inside the range, aside from the blue-to-white linear transition?
  • What should it do when selected range ends are hovered on?

Any of this could be a separate issue for after 1.0, it just seems that the date-picker design is a bit incomplete.

@alisonailea
Copy link
Contributor

I would defer to the designers here but there are use cases where a user would want to know what date they're hovering over after they have selected a date range. This goes double for someone interacting with the date picker via keyboard.

Same for highlighting a date within a selected range.

@Elijbet
Copy link
Contributor Author

Elijbet commented Jan 8, 2023

I would defer to the designers here but there are use cases where a user would want to know what date they're hovering over after they have selected a date range. This goes double for someone interacting with the date picker via keyboard.

Same for highlighting a date within a selected range.

Yeah, right now the only highlighting I see is a slightly darker text color, which I only noticed after I specifically looked for a distinction.

@ashetland
Copy link
Contributor

Hey folks, as of beta.97 it looks like all the hover states had been sorted out. Am I missing / misundestanding something?

CleanShot.2023-01-09.at.08.20.03.mp4

@SkyeSeitz
Copy link

Hey folks, as of beta.97 it looks like all the hover states had been sorted out. Am I missing / misundestanding something?

Tis upon clicking outside the date-picker

Screen.Recording.2023-01-09.at.11.13.57.AM.mov

@ashetland
Copy link
Contributor

Hey folks, as of beta.97 it looks like all the hover states had been sorted out. Am I missing / misundestanding something?

Tis upon clicking outside the date-picker

Screen.Recording.2023-01-09.at.11.13.57.AM.mov

I thought @Elijbet was asking about hover states not the square end-date.

@SkyeSeitz
Copy link

Hey folks, as of beta.97 it looks like all the hover states had been sorted out. Am I missing / misundestanding something?

Tis upon clicking outside the date-picker
Screen.Recording.2023-01-09.at.11.13.57.AM.mov

I thought @Elijbet was asking about hover states not the square end-date.

You are correct- sorry! It would help if I read the latest comment, not just the issue title 😅

@Elijbet
Copy link
Contributor Author

Elijbet commented Jan 9, 2023

Hey folks, as of beta.97 it looks like all the hover states had been sorted out. Am I missing / misundestanding something?
CleanShot.2023-01-09.at.08.20.03.mp4

Wow, ok. So turns out that because I had my contrast just one notch up, the grey highlighting completely faded out. I wasn't even aware it was there.

Screenshot 2023-01-09 at 12 22 37 PM

@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Jan 17, 2023
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Jan 17, 2023
@jcfranco
Copy link
Member

@Elijbet Was there anything pending for this? It looks like it's good to go. Let me know if I missed something.

Also, can you mark this as ready for review? I think we can skip draft reviews by default unless there's an explicit need for early feedback.

@jcfranco
Copy link
Member

One more thing, since date-picker-day is internal, can you use the public component in the PR title's scope:

fix(date-picker): end-range ...

?

@Elijbet
Copy link
Contributor Author

Elijbet commented Jan 19, 2023

@Elijbet Was there anything pending for this? It looks like it's good to go. Let me know if I missed something.

Also, can you mark this as ready for review? I think we can skip draft reviews by default unless there's an explicit need for early feedback.

There is more working code on this branch that isn't pushed up. Let me finalize it now and open if for review.

@Elijbet Elijbet changed the title fix(date-picker-day): end-range is now rounded and has the correct box-shadow fix(date-picker): end-range is now rounded and has the correct box-shadow Jan 19, 2023
@Elijbet Elijbet marked this pull request as ready for review January 20, 2023 00:25
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 20, 2023
@@ -221,7 +221,7 @@
}

:host([highlighted]:not([active]:focus)) .day {
@apply text-color-1 rounded-none;
@apply text-color-1 rounded-full;
Copy link
Member

Choose a reason for hiding this comment

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

@Elijbet Based on ☝️, is the util class still needed?

@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 20, 2023
@Elijbet Elijbet merged commit ed30588 into master Jan 20, 2023
@Elijbet Elijbet deleted the elijbet/5544-make-picker-end-date-a-circle branch January 20, 2023 03:16
benelan added a commit that referenced this pull request Jan 24, 2023
…negative-after-clear

* origin/master:
  build(deps): Bump gh-release from 6.0.4 to 7.0.1 (#6293)
  test(date): add test for dateToISO utility function (#6308)
  build: bump calcite-colors to latest (#6318)
  1.0.0-next.729
  fix(date-picker): end-range is now rounded and has the correct box-shadow (#6216)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants