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.

Sign up for GitHub

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

Jump to bottom

Remove inaccessible opacity from SettingsSection #5109

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Remove inaccessible opacity from SettingsSection #5109

merged 1 commit into from
Jan 22, 2024

Conversation

JuliaKirschenheuter
Copy link
Contributor

☑️ Resolves

Opacity is a bad choice for a11y in general. Let's remove it.

🖼️ Screenshots

🏚️ Before 🏡 After
Screenshot from 2024-01-22 11-21-09 Screenshot from 2024-01-22 11-21-21

🏁 Checklist

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

@JuliaKirschenheuter JuliaKirschenheuter added the 3. to review Waiting for reviews label Jan 22, 2024
@JuliaKirschenheuter JuliaKirschenheuter self-assigned this Jan 22, 2024
ShGKme
ShGKme reviewed Jan 22, 2024
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This has a design impact. IMO, it looked good with a bit gray text.

What about changing the color to:

color: var(--color-text-lighter);

image

Should be fine contrast-wise

image

image

@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/42941-remove-inaccessible-opacity branch from 13857d0 to d5a9db2 Compare January 22, 2024 13:11
susnux
susnux requested changes Jan 22, 2024
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.

Please do not use color-text-lighter

src/components/NcSettingsSection/NcSettingsSection.vue Outdated Show resolved Hide resolved
src/components/NcSettingsSection/NcSettingsSection.vue Outdated Show resolved Hide resolved
src/components/NcSettingsSection/NcSettingsSection.vue Outdated Show resolved Hide resolved
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/42941-remove-inaccessible-opacity branch from d5a9db2 to 1c20d8f Compare January 22, 2024 15:39
emoral435
emoral435 approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

Haven't tested this on my local dev env to check , but it looks good to me!

@emoral435 emoral435 force-pushed the fix/42941-remove-inaccessible-opacity branch from 1c20d8f to 406d698 Compare January 22, 2024 19:51
@susnux susnux added bug Something isn't working accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Jan 22, 2024
@susnux susnux added this to the 8.5.0 milestone Jan 22, 2024
@susnux susnux merged commit 08a7838 into master Jan 22, 2024
15 checks passed
@susnux susnux deleted the fix/42941-remove-inaccessible-opacity branch January 22, 2024 19:58
@Pytal Pytal mentioned this pull request Jan 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emoral435 emoral435 emoral435 approved these changes

@szaimen szaimen szaimen approved these changes

@ShGKme ShGKme ShGKme approved these changes

@susnux susnux susnux approved these changes

@Pytal Pytal Awaiting requested review from Pytal

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.5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@JuliaKirschenheuter @susnux @ShGKme @szaimen @emoral435