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

Properly calculate primary element based on background luminance #32509

Merged
merged 1 commit into from
May 31, 2022

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented May 20, 2022

Dark primary with dark theme Bright primary on light theme
2022-05-20_08-41 2022-05-20_08-41_1
2022-05-20_08-45_1 2022-05-20_08-45

supersede #32500
closes #32500

Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv skjnldsv added this to the Nextcloud 25 milestone May 20, 2022
@skjnldsv skjnldsv requested a review from a team May 20, 2022 06:48
@skjnldsv skjnldsv self-assigned this May 20, 2022
@skjnldsv skjnldsv requested review from PVince81, artonge, szaimen and CarlSchwan and removed request for a team May 20, 2022 06:48
@skjnldsv
Copy link
Member Author

For 24 and below, #32510

@artonge
Copy link
Contributor

artonge commented May 30, 2022

Ok code wise, but this does not respect the primary color.

Maybe elementColor should try to use a darker or a lighter color based on the primary color instead of defaulting to dark or light gray ?

Perhaps a question for @jancborchardt : Should we default to gray when the primary color does not have enough luminance compared to the background, or should we try to derive a new color from the primary color ? A third option would be to have a new setting to specify a primary color for dark themes.

@jancborchardt
Copy link
Member

We do need to fall back to gray when the contrast would be too low otherwise, e.g. for light yellow or plain white.

This seems to look good, but the folder icons on dark primary with dark theme should probably not be black as it's almost not visible?

@skjnldsv
Copy link
Member Author

Maybe elementColor should try to use a darker or a lighter color based on the primary color instead of defaulting to dark or light gray ?

I agree, but this is a different topic ;)
Improving this should be a followup.
This just fixes the behaviour by using something we created for this purpose.

Adapting the Util logic and methods can be discussed afterwards.
I think we need to sit down with the design team and make an update on how we use use to do things ans what we are not able to do with 25 (technically speaking)

@skjnldsv skjnldsv merged commit eca5e2d into master May 31, 2022
@skjnldsv skjnldsv deleted the fix/theming-colours-primary branch May 31, 2022 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants