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: Adapt the menubar to the primary color if a custom backgorund image has been set #36808

Closed
wants to merge 1 commit into from

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Feb 22, 2023

Steps to reproduce:

  • Set white as a primary color
  • Set a bright background image as the admin
  • Disable user theming (personal background image)

Before

App icons where white on white

After

App icons properly adapt to the primary color

@juliushaertl juliushaertl requested review from skjnldsv, szaimen, a team, ArtificialOwl and icewind1991 and removed request for a team February 22, 2023 11:46
@juliushaertl juliushaertl added this to the Nextcloud 26 milestone Feb 22, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Could you also add a use case for this in the cypress tests please?

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.

This doesnt seem to work if you choose a dark primary color. Also the app title in the heading doesnt seem to be inverted correctly...

@skjnldsv
Copy link
Member

skjnldsv commented Mar 2, 2023

We kinda had that discussion with #35482
If we want to apply a different rule following the primary, then we need to apply some overriding variables to the header element

@juliushaertl
Copy link
Member Author

Won't have time looking into that further this week, so If anyone wants to jump in feel free. Otherwise I'll check again somewhen next week.

@skjnldsv skjnldsv modified the milestones: Nextcloud 26, Nextcloud 27 Mar 2, 2023
@MichaIng
Copy link
Member

Related: #37379
Avatar and status icon get inverted as well, so it must be taken care that only app icons are inverted.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

One tiny comment, but all good otherwise

@skjnldsv
Copy link
Member

/backport to stable26

@skjnldsv

This comment was marked as resolved.

@szaimen
Copy link
Contributor

szaimen commented Apr 14, 2023

@skjnldsv the PR hasnt changed since I reviewed it so what shall I review here?

This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
@blizzz blizzz mentioned this pull request Nov 14, 2023
…age has been set

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

juliushaertl commented Dec 29, 2023

I revived this and pushed a cypress test.

This doesnt seem to work if you choose a dark primary color. Also the app title in the heading doesnt seem to be inverted correctly...

I could not reproduce this anymore

@juliushaertl
Copy link
Member Author

Thinking more about it i don't think this is the right approach, as long as we have no detection or setting for if the background is dark or bright, we can only make it work in either combination. The primary color is only a good indicator for inverting app icons if no background is used. Will close this.

@MichaIng MichaIng deleted the bugfix/noid/theme-light branch December 29, 2023 16:33
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Feb 23, 2024
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.

5 participants