Skip to content

Comments

Create outline for focus-within on RadioSwitch#4699

Merged
Pytal merged 1 commit intomasterfrom
fix/40706-Contrast_of_focused_checkbox_is_not_enough_
Nov 2, 2023
Merged

Create outline for focus-within on RadioSwitch#4699
Pytal merged 1 commit intomasterfrom
fix/40706-Contrast_of_focused_checkbox_is_not_enough_

Conversation

@JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Oct 26, 2023 •
edited
Loading

☑️ Resolves

  • Equalize styles for focused state: keyboard as well as mouse click
  • Apply styles for unchecked state

🖼️ Screenshots

Before After
image Screenshot from 2023-10-26 13-28-29
image Screenshot from 2023-10-26 13-26-04
Screenshot from 2023-10-26 13-34-43 Screenshot from 2023-10-26 13-29-24
Screenshot from 2023-10-26 13-35-20 Screenshot from 2023-10-26 13-29-14

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@JuliaKirschenheuter JuliaKirschenheuter self-assigned this Oct 26, 2023
@JuliaKirschenheuter JuliaKirschenheuter added the 3. to review Waiting for reviews label Oct 26, 2023
@susnux
Copy link
Contributor

susnux commented Oct 26, 2023

Does not work on the button style:

image

@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/40706-Contrast_of_focused_checkbox_is_not_enough_ branch from 485c886 to a123017 Compare October 27, 2023 08:01
@susnux
Copy link
Contributor

susnux commented Oct 27, 2023

Maybe something like outline-offset: -2px;?

@JuliaKirschenheuter
Copy link
Contributor Author

JuliaKirschenheuter commented Oct 27, 2023

Does not work on the button style:
image

Do you mean?:

Peek 2023-10-27 13-00

@susnux
Copy link
Contributor

susnux commented Oct 27, 2023

Do you mean?:

Yes but that is the SidebarTabs component, I am talking about this component NcCheckboxRadioSwitch in the "button style grouped" view:

vokoscreenNG-2023-10-27_13-05-32.mp4

@ShGKme
Copy link
Contributor

ShGKme commented Oct 27, 2023

Maybe something like outline-offset: -2px;?

I always used border+padding to make an outline inside an element, never heard about this property 🙈

@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/40706-Contrast_of_focused_checkbox_is_not_enough_ branch from a123017 to b4e15f3 Compare October 30, 2023 10:44
@AndyScherzinger AndyScherzinger added this to the 8.0.0 milestone Oct 30, 2023
Pytal
Pytal reviewed Oct 30, 2023
View reviewed changes
susnux
susnux approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Looks good overall, but I agree with @Pytal (while it is not a blocker for me)

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Oct 31, 2023

@JuliaKirschenheuter focus-within is not needed and causes an unwanted behavior of showing the outline on click. We don't want that. I took the liberty to push a fix. Please review @Pytal @susnux @JuliaKirschenheuter
Thanks :)

marcoambrosini
marcoambrosini previously requested changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

blocking this until we reach a proper solution

@JuliaKirschenheuter JuliaKirschenheuter added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels Oct 31, 2023
@JuliaKirschenheuter
Copy link
Contributor Author

JuliaKirschenheuter commented Oct 31, 2023

@JuliaKirschenheuter focus-within is not needed and causes an unwanted behavior of showing the outline on click. We don't want that.

I have to clarify that. Probably i had mistakes in thinking.

@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as draft October 31, 2023 07:48
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/40706-Contrast_of_focused_checkbox_is_not_enough_ branch from 74c6405 to d791a54 Compare November 2, 2023 14:00
@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as ready for review November 2, 2023 14:01
@JuliaKirschenheuter JuliaKirschenheuter added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Nov 2, 2023
susnux
susnux approved these changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Checked again, works!

@susnux susnux requested a review from Pytal November 2, 2023 16:48
@Pytal Pytal force-pushed the fix/40706-Contrast_of_focused_checkbox_is_not_enough_ branch from 67983b9 to bfad217 Compare November 2, 2023 22:18
@Pytal Pytal merged commit 3f605f3 into master Nov 2, 2023
@Pytal Pytal deleted the fix/40706-Contrast_of_focused_checkbox_is_not_enough_ branch November 2, 2023 22:22
@skjnldsv
Copy link
Contributor

skjnldsv commented Nov 3, 2023 •
edited
Loading

Please set appropriate bug or enhancement labels next time 👍

@skjnldsv skjnldsv added bug Something isn't working accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Nov 3, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

@susnux susnux susnux approved these changes

@ShGKme ShGKme Awaiting requested review from ShGKme

@szaimen szaimen Awaiting requested review from szaimen

@marcoambrosini marcoambrosini Awaiting requested review from marcoambrosini

+1 more reviewer

@Pytal Pytal Pytal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JuliaKirschenheuter JuliaKirschenheuter

Labels

3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

[BITV]: Contrast of focused checkbox is not enough

7 participants

@JuliaKirschenheuter @susnux @ShGKme @marcoambrosini @skjnldsv @AndyScherzinger @Pytal