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(theming): fix incorrectly inverted favicons #44119

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Mar 11, 2024

Reason: invertTextColor has changed and is not now incorrectly inverting icons even with the default primary color. This now switches to isBrightColor which more correctly returns what we need here.

Before After with default primary color After with white as primary color After with black as primary color
image image image image

@szaimen szaimen added bug design Design, UI, UX, etc. 2. developing Work in progress feature: theming labels Mar 11, 2024
@szaimen szaimen added this to the Nextcloud 29 milestone Mar 11, 2024
Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 11, 2024
@szaimen szaimen marked this pull request as ready for review March 11, 2024 10:09
@szaimen szaimen requested review from jancborchardt, susnux, marcoambrosini, nimishavijay, a team, Pytal and emoral435 and removed request for a team March 11, 2024 10:10
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Works but not sure if this is intended, because the background color is not the primary color.
The reason the default looks odd is that for our default color we use the accessible color instead (there is a if color === ... then instead use other color).

@emoral435
Copy link
Contributor

The reason the default looks odd is that for our default color we use the accessible color instead

If that is the case, then should we change the colors? I like the change visually, but if it reverts accessibility, should it not be kept?

@susnux
Copy link
Contributor

susnux commented Mar 11, 2024

but if it reverts accessibility, should it not be kept?

No in this case it is only the icon. But until we (maybe) get #42977 in we use a hack in DefaultTheme and in Capabilities that changes the primary color if it is Nextcloud blue to some slighly darker blue -> results then in white text color.

@emoral435
Copy link
Contributor

But until we (maybe) get #42977 in we use a hack in DefaultTheme and in Capabilities that changes the primary color if it is Nextcloud blue to some slighly darker blue -> results then in white text color.

I see! I like both solutions, but it seems that this would be more of a bandaid fix, so we might as well wait for the above PR to (hopefully?) be merged, seems much cleaner :) LGTM for now then!

@susnux susnux merged commit 53220d0 into master Mar 11, 2024
163 checks passed
@susnux susnux deleted the enh/noid/inverted-favicons branch March 11, 2024 15:30
@Altahrim Altahrim mentioned this pull request Mar 12, 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.

Thanks for fixing that! :)

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 bug design Design, UI, UX, etc. feature: theming
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants