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

Header icons need to invert to black/dark on specific preset background images (like the Dashboard does) #33969

Closed
jancborchardt opened this issue Sep 8, 2022 · 14 comments · Fixed by #34665
Assignees
Labels
4. to release Ready to be released and/or waiting for tests to finish 25-feedback bug high ui-refresh-feedback
Milestone

Comments

@jancborchardt
Copy link
Member

The Dashboard already does this correctly, and as discussed with @juliushaertl we should use the same logic for the other pages. Specifically, this applies to these 4 backgrounds:

  • bernie-cetonia-aurata-take-off-composition.jpg
  • lali-masriera-yellow-bricks.jpg
  • rawpixel-pink-tapioca-bubbles.jpg
  • tommy-chau-lion-rock-hill.jpg

The logic seems to be with the --primary-invert-if-bright variable as far as I can see.

Correct in Dashboard – I would additionally say that we need to invert the logo as well (only if it is our default Nextcloud logo though) cc @karlitschek
image

Needs to be adjusted in the other apps. I assume with Julius’ move of the header to Vue this might be easier?
image

@jancborchardt jancborchardt added bug 1. to develop Accepted and waiting to be taken care of ui-refresh-feedback labels Sep 8, 2022
@jancborchardt jancborchardt added this to the Nextcloud 25 milestone Sep 8, 2022
@jancborchardt jancborchardt mentioned this issue Sep 8, 2022
64 tasks
@szaimen
Copy link
Contributor

szaimen commented Sep 9, 2022

@CarlSchwan could you maybe also have a fast look into this? I think it is pretty bad...

@skjnldsv
Copy link
Member

Once #34272 is done, we can easily adjust --primary-invert-if-bright accordingly
I can handle it if no one is diving into this. :)

cc @Pytal

@Pytal
Copy link
Member

Pytal commented Sep 28, 2022

Preset colors of shipped backgrounds are used as primary in #34298

If this touches the same code then it can be handled by @skjnldsv after the above PR

@skjnldsv
Copy link
Member

skjnldsv commented Oct 5, 2022

Fixed with #34298

@skjnldsv skjnldsv closed this as completed Oct 5, 2022
@Pytal
Copy link
Member

Pytal commented Oct 12, 2022

Fixed on dashboard and appearance pages but not globally like on the files page

The --primary-invert-if-bright variable needs to be adjusted then @skjnldsv?

@skjnldsv
Copy link
Member

Isn't the primary-invert-if-bright already using the primary colour?
Why does it needs extra work then?

@skjnldsv
Copy link
Member

'--primary-invert-if-bright' => $this->util->invertTextColor($this->primaryColor) ? 'invert(100%)' : 'no',

@skjnldsv
Copy link
Member

Ah, this should be properly fixed by #34437 I guess

@szaimen
Copy link
Contributor

szaimen commented Oct 12, 2022

Ah, this should be properly fixed by #34437 I guess

no, not for custom background. It will probably only get fixed with #34512

@skjnldsv
Copy link
Member

@szaimen this issue is not about custom background

@szaimen
Copy link
Contributor

szaimen commented Oct 12, 2022

Ah I see.

@Pytal
Copy link
Member

Pytal commented Oct 12, 2022

Ah, this should be properly fixed by #34437 I guess

👌

@skjnldsv skjnldsv self-assigned this Oct 13, 2022
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 1. to develop Accepted and waiting to be taken care of labels Oct 13, 2022
@skjnldsv
Copy link
Member

Merged and fixed

@Pytal
Copy link
Member

Pytal commented Oct 18, 2022

Seems that the header icons on the files page and others (excluding dashboard and appearance pages) are still not inverted

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish 25-feedback bug high ui-refresh-feedback
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants