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

[stable27] fix(theming): stable 27 disable accessible color config switch #45422

Merged
merged 1 commit into from
May 21, 2024

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented May 21, 2024

Fix #39551

Only for 27! 28 and above have a different handling of themes and is fixed

Apply this fix

  1. add '27-disable-accessible-theming-color' => true, to your config.php
  2. delete the global folder on your data/appdata_xxxx/theming directory (no need to delete the theming folder)
  3. run an occ files:scan-app-data theming

How to reproduce the issue

  1. use a non accessible color like #004352
  2. load the files app
Before After
localhost_8081_index php_apps_files__dir=_ fileid=2 (1) localhost_8081_index php_apps_files__dir=_ fileid=2

Signed-off-by: John Molakvoæ <skjnldsv@users.noreply.github.com>
@skjnldsv skjnldsv added bug design Design, UI, UX, etc. 3. to review Waiting for reviews feature: theming labels May 21, 2024
@skjnldsv skjnldsv added this to the Nextcloud 27.1.10 milestone May 21, 2024
@skjnldsv skjnldsv requested review from AndyScherzinger, artonge and a team May 21, 2024 14:25
@skjnldsv skjnldsv self-assigned this May 21, 2024
@skjnldsv skjnldsv requested review from Fenn-CS and szaimen and removed request for a team May 21, 2024 14:25
@szaimen
Copy link
Contributor

szaimen commented May 21, 2024

28 and above have a different handling of themes and is fixed

I fear it is not, seems to behave completely the same, just tested (it still adjusts the color to an accessible one)...


primary color (black):
image
folder color (grey):
image

@artonge
Copy link
Contributor

artonge commented May 21, 2024

folder color (grey):

But that is expected, no? Black on black does not work for accessibility.

@szaimen
Copy link
Contributor

szaimen commented May 21, 2024

folder color (grey):

But that is expected, no? Black on black does not work for accessibility.

yes, this is also what I am understanding but seems like the customer complains about exactly this, no?

@AndyScherzinger
Copy link
Member

yes, this is also what I am understanding but seems like the customer complains about exactly this, no?

No the complaint is the before image which shows the folder color used for dark and light

@skjnldsv
Copy link
Member Author

skjnldsv commented May 21, 2024

yes, this is also what I am understanding but seems like the customer complains about exactly this, no?

No the complaint is the before image which shows the folder color used for dark and light

28 with the color above:
image

EDIT: @AndyScherzinger the colour shift is till being done, but the computation is different to be accessible.
We might have to check with the customer again that they're ok with it

@szaimen
Copy link
Contributor

szaimen commented May 21, 2024

yes, this is also what I am understanding but seems like the customer complains about exactly this, no?

No the complaint is the before image which shows the folder color used for dark and light

I see. 👍

@AndyScherzinger
Copy link
Member

We might have to check with the customer again that they're ok with it

Yes, we might. The main issue is with the light theme though. The light folder icons in your before screenshot are also used in light mode on 27...

So the main issue to be addressed with this fix is:
2024-05-21 16_43_47-Files - Cloud – Mozilla Firefox

@AndyScherzinger
Copy link
Member

AndyScherzinger commented May 21, 2024

@skjnldsv how would the light theme look like in 28 with that color? That is the main concern with the given color code

@skjnldsv
Copy link
Member Author

@skjnldsv how would the light theme look like in 28 with that color? That is the main concern with the given color code

image
Because we fixed the computation on the client side, so we now which theme is active 👍

@AndyScherzinger
Copy link
Member

AndyScherzinger commented May 21, 2024

Sweet, so v28 looks good to me 👍
Can't tell if this works fine with they background image as already mentioned by @artonge to me.

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 21, 2024
@skjnldsv skjnldsv merged commit 51adff2 into stable27 May 21, 2024
37 of 38 checks passed
@skjnldsv skjnldsv deleted the skjnldsv-patch-1 branch May 21, 2024 15:30
@skjnldsv skjnldsv mentioned this pull request May 22, 2024
10 tasks
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 bug design Design, UI, UX, etc. feature: theming
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

4 participants