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

theming: Separate primary and background colors - fix the header menu colors #42977

Merged
merged 17 commits into from
May 21, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 20, 2024

Summary

This will fix one of the main issues we face with our theming: Currently primary color used to highlight elements is mixed and directly connected to the background color.

So the goal is to have two different colors:

  1. primary to highlight elements - meaning used as an accent color
  2. background to be used as background on the body or fallback if images failes to load, moreover this will be used to calculate the header menu text color.

To make this possible this PR is sadly bigger then I hoped, it contains following changes:

  1. Add background / primary color for custom images
    Previously we had a primary color predefined for every shipped background image, but not for custom images so that users had to select a color manually. As preparation for the other steps I added a function to automatically calculate the mean color of the image so it can be used as the primary / background color.
  2. Separate background and primary color
    While the primary color is intended to highlight elements, it can not always be used as the background color. So now primary is independent from background a user sets, the background color is, if not set directly, calculated as the mean color of the background image. That color is then used as a fallback if the background image could not be loaded and for calculating the color of the text used on the app menu and dashboard (they render directly on the background).
  3. Adjust dashboard and the header menu to use that new color instead of primary
  4. Adjust theming setting to configure primary and background color
  5. Fixed our provided apps that have header menus to use color variables instead of the infamous color-filter.

TODO

  • Tests
  • How to implement migration? IRepairStep? All users need to copy background_color to primary_color user value

Checklist

@susnux susnux requested a review from provokateurin as a code owner January 20, 2024 02:37
@susnux susnux marked this pull request as draft January 20, 2024 02:38
@susnux susnux added enhancement design Design, UI, UX, etc. 2. developing Work in progress feature: theming labels Jan 20, 2024
@susnux susnux added this to the Nextcloud 29 milestone Jan 20, 2024
@susnux susnux changed the title Theming/create color from background theming: Separate primary and background colors - fix the header menu colors Jan 20, 2024
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@susnux susnux force-pushed the theming/create-color-from-background branch 2 times, most recently from 76f6d54 to c0f68a9 Compare January 20, 2024 14:53
@susnux susnux added the pending documentation This pull request needs an associated documentation update label Jan 22, 2024
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.

@susnux in which cases was the current state specifically a problem? :) I was even thinking we can phase out color-primary-element in favor of just color-primary – since for our default that is now unified to the darker blue.

I really like your point 1) for example so things are automatically generated
4) though seems to make stuff more complex, not sure we need the separation. But that's why I'm asking. :) cc @juliushaertl

@nickvergessen
Copy link
Member

I was even thinking we can phase out color-primary-element in favor of just color-primary – since for our default that is now unified to the darker blue.

Please keep our clients in mind that rely on these values and need a non-manipulated primary color, so they can do their own math using material design on Android and ... on iOS to generate colors matching their standards.

@juliusknorr
Copy link
Member

juliusknorr commented Jan 22, 2024

  1. though seems to make stuff more complex, not sure we need the separation. But that's why I'm asking

I think it would be good to have this separation, especially for cases where people disable user background theming (like in corporate environments) and then want to have a static background image (+color). Icon inversion in the header menu needs to be based on the background color or image and not the primary element color so splitting that makes a lot of sense to me.

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.

@nickvergessen @juliushaertl right, if you say it makes sense let’s go with it. :) I can’t judge the code though.

@Pytal Pytal mentioned this pull request May 17, 2024
8 tasks
@szaimen szaimen requested a review from Pytal May 18, 2024 08:53
@susnux susnux force-pushed the theming/create-color-from-background branch 2 times, most recently from 1d0e4c6 to 3e9ac19 Compare May 20, 2024 22:26
susnux added 17 commits May 21, 2024 20:36
While the primary color is intended to highlight elements,
it can not always be used as the background color.
So now primary is independent from background a user set,
the background color is, if not set directly, calculated as the mean color
of the background image. That color is then used as a fallback if the
background image could not be loaded and for calculating the color of the text used on the app menu and dashboard (they render directly on the background).

Signed-off-by: Ferdinand Thiessen <[email protected]>
fix(UnifiedSearch): Adjust to new background color

Signed-off-by: Ferdinand Thiessen <[email protected]>
…und in admin settings

Signed-off-by: Ferdinand Thiessen <[email protected]>
…al background image

Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the theming/create-color-from-background branch from 3e9ac19 to 651afb8 Compare May 21, 2024 18:42
@susnux susnux merged commit 576e249 into master May 21, 2024
155 checks passed
@susnux susnux deleted the theming/create-color-from-background branch May 21, 2024 19:16
@szaimen
Copy link
Contributor

szaimen commented May 21, 2024

🎉🎉🎉🎉🎉🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. enhancement feature: theming pending documentation This pull request needs an associated documentation update
Projects
None yet
7 participants