-
Notifications
You must be signed in to change notification settings - Fork 77
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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: |
@@ -221,7 +221,7 @@ | |||
} | |||
|
|||
:host([highlighted]:not([active]:focus)) .day { | |||
@apply text-color-1 rounded-none; | |||
@apply text-color-1 rounded-full; |
There was a problem hiding this comment.
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
I have a couple of design questions on
Any of this could be a separate issue for after 1.0, it just seems that the |
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. |
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 |
Tis upon clicking outside the 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 😅 |
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. |
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. |
@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. |
One more thing, since
? |
There is more working code on this branch that isn't pushed up. Let me finalize it now and open if for review. |
@@ -221,7 +221,7 @@ | |||
} | |||
|
|||
:host([highlighted]:not([active]:focus)) .day { | |||
@apply text-color-1 rounded-none; | |||
@apply text-color-1 rounded-full; |
There was a problem hiding this comment.
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?
Related Issue: #5544
Summary
Improvements to
date-picker-day
styling include:end-range
is now roundedbox-shadow
dir="ltr"
anddir="rtl"