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(styles): add focus color on datepicker component #2061

Conversation

davidritter-dotcom
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2023

⚠️ No Changeset found

Latest commit: 57efb7d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@swisspost-bot
Copy link
Contributor

swisspost-bot commented Oct 10, 2023

Preview environment ready: https://preview-2061--swisspost-web-frontend.netlify.app
Preview environment ready: https://preview-2061--swisspost-design-system-next.netlify.app

Copy link
Contributor

@imagoiq imagoiq left a comment

Choose a reason for hiding this comment

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

Also, don't forget to lint. I would advise for doing it automatically with your IDE to avoid forgetting about it.

Comment on lines 101 to 103
&:focus-visible{
outline: forms.$input-focus-color solid forms.$input-focus-outline-thickness;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

With keyboard navigation, it doesn't work (check Firefox as the default color is blue, it's more obvious).

still missing on the navigation buttons
Comment on lines 149 to 151
&:focus-visible {
outline: forms.$input-focus-outline-thickness solid var(--post-contrast-color);
}
Copy link
Contributor

@imagoiq imagoiq Oct 19, 2023

Choose a reason for hiding this comment

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

This one is not compliant with WCAG. Check this article for example to better understand: https://www.sarasoueidan.com/blog/focus-indicators/

You need the same style as buttons.

Copy link
Contributor

@imagoiq imagoiq left a comment

Choose a reason for hiding this comment

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

Need another style for the trigger button.

Copy link
Member

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

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@gfellerph
Copy link
Member

@davidritter-dotcom implement with the current definition in Figma.

@gfellerph
Copy link
Member

@davidritter-dotcom design is ready. This is no longer blocked.

Copy link
Member

@gfellerph gfellerph left a comment

Choose a reason for hiding this comment

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

Fix linting errors and update according to current Design. If that's done, please re-request review.

@davidritter-dotcom
Copy link
Contributor Author

Waiting for #2855 because of the change in the foucs-style mixin.

@imagoiq imagoiq removed their request for review April 3, 2024 13:47
Copy link
Member

@gfellerph gfellerph left a comment

Choose a reason for hiding this comment

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

There some points I noticed that don't seem to be working right now

  • Focus ring should not be visible when clicking with a mouse,
  • Focus styles are not visible if a date is selected (select a date and open the picker again, the focus ring should be visible when interacting with keyboard)
  • When the picker is open, the date field (and in the demo some more components) get a focus ring as well image.
    This should not be the case

@oliverschuerch
Copy link
Contributor

@davidritter-dotcom: This task is in progress quite long now. Do you need help with something?
Do you need preciser instructions from one of the reviewer (@imagoiq and @gfellerph)?

Let me know if we can help.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@davidritter-dotcom
Copy link
Contributor Author

The focus on the select itself gets fixed when a decision is reached on how to apply focus styles to form-select and form-control.

@gfellerph gfellerph requested review from imagoiq and removed request for imagoiq April 26, 2024 07:19
@davidritter-dotcom davidritter-dotcom merged commit 532cb6f into main Apr 30, 2024
9 checks passed
@davidritter-dotcom davidritter-dotcom deleted the 1807-datepicker-component-set-focus-outline-color-on-calendar-button branch April 30, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Datepicker component] Set focus outline color on calendar button
5 participants