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

Use the color-primary-element* variables #1545

Closed
wants to merge 1 commit into from

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented May 12, 2023

Explanation: the color-primary variables are not to be used in components because the introduce problems with high-contrast primary colors. Fix this by using the primary-element variables instead.

Signed-off-by: Simon L <szaimen@e.mail.de>
@@ -5,7 +5,7 @@
fill: var(--color-warning);
}
&--white {
fill: var(--color-primary-text);
fill: var(--color-primary-element-text);
Copy link
Member

Choose a reason for hiding this comment

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

Those 2 colors can never diverge, both are either black or white and that does not change by the "-element" cut of :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Then it is probably fine. However I wonder if we should still normalize this here as well.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC the problem goes even further because it still assumes it would be on primary color from the topbar, but it is on the background image, so the white might not be visible properly and instead of assigning a color we need to fall back to the normal font color, but as said. needs more testing to have clear results for all colors and states at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see

@nickvergessen
Copy link
Member

Will have a look next week, but I think this is not needed (as per above).

@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@szaimen
Copy link
Contributor Author

szaimen commented Jun 20, 2023

I guess we can close this?

@nickvergessen nickvergessen deleted the enh/noid/use-primary-element branch June 21, 2023 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants