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(theming): change color button contrast #42552

Merged
merged 5 commits into from
Jan 25, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jan 2, 2024

Summary

Use NcButton as a trigger for changing color to make the text always contrast.

Note: there is no pair of text colors that is contrast enough on any background. E.g. in some gray of blue backgrounds, both white and black text is not contrast enough. So we desided to use fixed background for the text.

TODO

  • User Settings: Use NcButton on the color preview instead of plain text to ensure it is well-visible
  • Admin Settings: Separate NcButton and the color preview
  • Adjust tests and use more semantic selectors

Screenshots

🏚️ Before 🏡 After
image image
image image
image image

Other visual changes

Popover is now shown on the button

image

There is no interactivity on the colored rectangle. Making it interactive would need to get rid of using NcButton and copy its styles.

change-color-hover

Checklist

@ShGKme ShGKme added this to the Nextcloud 29 milestone Jan 2, 2024
@ShGKme ShGKme self-assigned this Jan 2, 2024
@ShGKme ShGKme force-pushed the fix/41849/theming--change-color-button-contrast branch from 28a26e5 to b70f9e9 Compare January 2, 2024 15:57
@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 2, 2024
@ShGKme ShGKme marked this pull request as ready for review January 2, 2024 15:58
@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 2, 2024

/backport to stable28

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Nice and simple solution, looks good design-wise.

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 8, 2024

Seems that this change is actually required for a11y only if there is still Nextcloud Blue in the palette. Which we don't use anywhere anymore as a primary color...

@nextcloud/designers do you think this is fine to merge regardless of a11y requirements, or better to avoid it, if we decide to remove the problematic color from the palette?

@szaimen
Copy link
Contributor

szaimen commented Jan 9, 2024

I think it is fine to merge it. This should probably also solve cases with custom colors...

@ShGKme ShGKme force-pushed the fix/41849/theming--change-color-button-contrast branch from b70f9e9 to f9f0dc7 Compare January 9, 2024 15:19
@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 9, 2024

Rebased onto main, fixed Cypress e2e tests.

@ShGKme ShGKme force-pushed the fix/41849/theming--change-color-button-contrast branch from f9f0dc7 to 1ccc64b Compare January 16, 2024 17:52
@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 16, 2024

I forgot that we have the same button with the same issue also on Admin settings.

I separated the button and color view and added icon to be inline with other buttons here.
Re-requesting designers to re-approve. Sorry for the double work 👉👈

@ShGKme ShGKme force-pushed the fix/41849/theming--change-color-button-contrast branch from 1ccc64b to 4151bd7 Compare January 16, 2024 18:38
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

LGTM from design perspective :)

@susnux
Copy link
Contributor

susnux commented Jan 17, 2024

@ShGKme can you resolve the conflicts?

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 17, 2024

@ShGKme can you resolve the conflicts?

There is a Cypress issue that I haven't resolved yet.

@ShGKme ShGKme force-pushed the fix/41849/theming--change-color-button-contrast branch from 4151bd7 to ddeb8e6 Compare January 17, 2024 21:40
@susnux susnux force-pushed the fix/41849/theming--change-color-button-contrast branch from ddeb8e6 to 5325a8e Compare January 18, 2024 01:55
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
…view

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/41849/theming--change-color-button-contrast branch from 5325a8e to 6215814 Compare January 24, 2024 13:37
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the fix/41849/theming--change-color-button-contrast branch from 49e85f8 to 5d61b80 Compare January 25, 2024 15:04
@susnux susnux merged commit 342d753 into master Jan 25, 2024
45 checks passed
@susnux susnux deleted the fix/41849/theming--change-color-button-contrast branch January 25, 2024 15:48
@JuliaKirschenheuter
Copy link
Contributor

/backport to stable28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants