-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix(theming): change color button contrast #42552
Conversation
28a26e5
to
b70f9e9
Compare
/backport to stable28 |
There was a problem hiding this 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.
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? |
I think it is fine to merge it. This should probably also solve cases with custom colors... |
b70f9e9
to
f9f0dc7
Compare
Rebased onto main, fixed Cypress e2e tests. |
f9f0dc7
to
1ccc64b
Compare
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. |
1ccc64b
to
4151bd7
Compare
There was a problem hiding this 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 :)
@ShGKme can you resolve the conflicts? |
There is a Cypress issue that I haven't resolved yet.
|
4151bd7
to
ddeb8e6
Compare
ddeb8e6
to
5325a8e
Compare
Signed-off-by: Grigorii K. Shartsev <[email protected]>
Signed-off-by: Grigorii K. Shartsev <[email protected]>
…view Signed-off-by: Grigorii K. Shartsev <[email protected]>
Signed-off-by: Grigorii K. Shartsev <[email protected]>
5325a8e
to
6215814
Compare
Signed-off-by: Ferdinand Thiessen <[email protected]>
49e85f8
to
5d61b80
Compare
/backport to stable28 |
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
NcButton
on the color preview instead of plain text to ensure it is well-visibleNcButton
and the color previewScreenshots
Other visual changes
Popover is now shown on the button
There is no interactivity on the colored rectangle. Making it interactive would need to get rid of using
NcButton
and copy its styles.Checklist