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

DatePicker: better hover/focus styles #65117

Merged
merged 7 commits into from
Sep 11, 2024
Merged

DatePicker: better hover/focus styles #65117

merged 7 commits into from
Sep 11, 2024

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Sep 6, 2024

What?

DatePicker: fix buggy hover styles and improve focus styles for selected day, add high-contrast mode styles.

Why?

Fix bugs, improve accessibility and UI polish

How?

Tweaking the day button styles.

Testing Instructions

  • Open the DatePicker in storybook
  • Make sure that, when hovering over the selected day, the color of the button remains white
  • Make sure that, when focusing the selected day, a more clear focus ring is shown
  • Make sure that, when in high-contrast more, users can effectively understand what is the selected day
    • In chrome, this can be emulated using dev tools by setting the forced-colors: active media feature

Screenshots or screencast

Before After
Kapture.2024-09-06.at.12.07.46.mp4
date-picker-after.mp4

@ciampo ciampo requested a review from ajitbohra as a code owner September 6, 2024 10:08
Copy link

github-actions bot commented Sep 6, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: t-hamano <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ciampo ciampo requested review from noisysocks, t-hamano and a team September 6, 2024 10:09
@ciampo ciampo self-assigned this Sep 6, 2024
@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Sep 6, 2024
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

When the calendar uses a list of events, and the selected date has an event, it still looks weird:

Screenshot 2024-09-06 at 14 53 30

This looks like something we should address in this PR.

@ciampo
Copy link
Contributor Author

ciampo commented Sep 6, 2024

Well spotted, @tyxla  — the bug that you highlighted was caused by my code. I had initially used the ::before pseudo-element for the high-contrast mode, but as you found out, that pseudo-element is already used to display the "event dot".
Switching to the ::after pseudo-element for high-contrast styles seems to fix the issue.

I also took the opportunity to make the "event dot" work in high contrast mode, too.

@ciampo ciampo requested a review from tyxla September 6, 2024 16:19
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Great catch, thanks 👍

packages/components/src/date-time/date/styles.ts Outdated Show resolved Hide resolved
packages/components/src/date-time/date/styles.ts Outdated Show resolved Hide resolved
packages/components/src/date-time/date/styles.ts Outdated Show resolved Hide resolved
packages/components/src/date-time/date/styles.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

It works as expected in Windows high contrast mode.

This may be a minor thing, but is it possible to fix the misaligned event dots when a button gets focus? This issue doesn't seem to occur on trunk:

c103c38d06110ee9214f9695eebac5d2.mp4

@ciampo
Copy link
Contributor Author

ciampo commented Sep 9, 2024

is it possible to fix the misaligned event dots when a button gets focus? This issue doesn't seem to occur on trunk

I'm aware of that little jump — it's a consequence of showing an extra "white" focus ring to better indicate when a selected day is focused.

I'll try to apply Lena's suggestion and see how that goes

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM 👍 The bug has been addressed.

Should be good to go once prior feedback is addressed.

@ciampo ciampo force-pushed the fix/date-picker/day-hover branch from ec7cff0 to db54470 Compare September 11, 2024 07:50
@ciampo
Copy link
Contributor Author

ciampo commented Sep 11, 2024

I addressed all pending feedback — the component is testing well for me

Kapture.2024-09-11.at.09.52.05.mp4

Thank you all for the review, I'll merge once CI checks are 🟢

@ciampo ciampo enabled auto-merge (squash) September 11, 2024 07:54
@ciampo ciampo merged commit 20ca80f into trunk Sep 11, 2024
61 checks passed
@ciampo ciampo deleted the fix/date-picker/day-hover branch September 11, 2024 08:24
@github-actions github-actions bot added this to the Gutenberg 19.3 milestone Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants