[5.0] Allow OS color scheme override #42209

dgrammatiko · 2023-10-23T17:34:07Z

Pull Request for Issue #42134 .

Summary of Changes

introduce a selector for dark/light or OS controlled colour scheme
Reflect the state to the HTML element with a data-forced-theme and a data-theme="light|dark"

TODO:

Create a new _root.scss so that the theme follows the states of the above HTML element states. (ie like: https://github.com/dgrammatiko/muta/blob/b675f730671e3dcf43e2c1331c6f7d80fcc6cb2c/media_source/templates/administrator/muta/scss/_root.scss#L1-L3)

Testing Instructions

Check that in the Atum template the HTML element reflects the state of the User settings

Actual result BEFORE applying this Pull Request

Atum Template is always set to the OS Color Scheme

Expected result AFTER applying this Pull Request

Atum Template can override the OS Color Scheme

Link to documentations

Please select:

Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed

Signed-off-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev>

richard67 · 2023-10-23T18:06:29Z

Why 2 binary switches? Why not one ternary option?

value 0 „follow OS“ (= default)
value 1 „light mode“
value 2 „dark mode“

dgrammatiko · 2023-10-23T18:10:48Z

Why 2 binary switches? Why not one ternary option?

Not a particular reason, I can change the code to that. A minor thing is that BS supports more than dark/light themes so limiting the options and also making the addition of new themes harder might not be the best option (future wise).

richard67 · 2023-10-23T18:26:38Z

Let’s see if more opinions drop in regarding what’s better from a user point of view.

Other question: Shall we close the issue as we have a PR, or shall we leave it open until the ToDo is done?

Signed-off-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev>

administrator/language/en-GB/com_users.ini

dgrammatiko · 2023-10-23T18:31:25Z

Shall we close the issue as we have a PR, or shall we leave it open until the ToDo is done?

Meh, keep it open so people have a place to express their legitimate concerns 😀

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

administrator/language/en-GB/com_users.ini

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

brianteeman · 2023-10-23T18:34:49Z

will this not be dependent on the changes in #42010

dgrammatiko · 2023-10-23T18:36:46Z

will this not be dependent on the changes in #42010

No, as I wrote I didn't do any SCSS changes here, I'm just passing the State to the HTML Element so it will not conflict with the other PR

richard67 · 2023-10-23T18:55:14Z

The description of this PR should be adapted to the change to one tristate (list) parameter.

wilsonge · 2023-10-23T19:16:35Z

We're going to need two different compilations of the css file to support this and bundle that (plus admin template overrides in potentially two places and code to support that if you're setting bonus CSS vars - which presumably would be part of this - but missing). I really don't see the need. More people have moaned about this because they don't like the design which should be fixed in the other PR than because they actually want a dark mode browser with a bright site. I think we should focus efforts on making the styling of dark mode better than the all black thing I did (obviously I'm not the worlds best designer) than adding extra options and overhead.

dgrammatiko · 2023-10-23T19:31:05Z

We're going to need two different compilations of the css file to support this and bundle that.

Nope, check the link in the description, there is only one cas file!

HLeithner · 2023-10-23T19:31:44Z

why don't we go the bootstrap why?
Adding the switch functionality to Joomla.themeSelector javascript class similar what's used by bootstrap saving the manual switch in the browser local storage and allow to use Javascript Options be loaded from the use personal settings?

btw. I wouldn't separated front and backend, we already have to many switchs.

dgrammatiko · 2023-10-23T19:48:26Z

why don't we go the bootstrap way?

The bootstrap way:

Joomla5:

The difference is that the state is saved in the DB which (for the logged in users) doesn't need writing in the users HD (local storage as cookies are data written in the disk). The JS switch could save the state to either: local storage, session storage, indexDB, cookie which only session storage is on the RAM (so the other require permission).
In short saving the state in the DB is one of the possible solutions but not the only one (ie: in Muta I'm using cookies).

btw. I wouldn't separated front and backend, we already have to many switchs.

Can do

richard67 · 2023-10-23T20:24:24Z

administrator/templates/atum/component.php

+// Getting user accessibility settings
+$user            = $app->getIdentity();
+$userScheme      = $user->getParam('admin_prefers_color_scheme', '0');
+$forcedSchemeAtr = $forcedScheme ? 'data-forced-theme ' : '';


Where does $forcedScheme come from? Can it be the logic here needs a change after the change to tristate?

Hmm, I think the above line 53 just needs to be removed. Same for the other PHP files of course.

It supposed to be forcedSchemeAtr = '';

richard67 · 2023-10-24T07:11:41Z

administrator/templates/atum/error_full.php

+            --link-color: ' . $linkColor . ';
+            --link-color-rgb: ' . $r . ',' . $g . ',' . $b . ';
+            --template-special-color: ' . $this->params->get('special-color', '#001B4C') . ';
+        }');


I think the "code style" change here should be reverted. It's in the inline CSS not the PHP code, so I think the tabs are ok, also for consistency with the other template PHP files.

bembelimen · 2023-10-24T16:02:12Z

I like the switch, but I think we should not create our own wheel again and just using what we already have in Joomla. Also only one attribute is needed but we have to change this parameter to e.g. "data" (or something else and fix the broken and wrongly added definitions).

You can test this by adding data-bs-theme="dark|light" attribute to the html tag in joomla backend. So please don't add another dark/light mode layer on top of the existing one.

dgrammatiko · 2023-10-24T16:22:33Z

we should not create our own wheel

It's just a DB value for a given key, people/devs could use it or not. Ie if I want to roll out something that is not requiring permissions (due to EU law about asking before writing to users HD) then such a value is the only way (also it doesn't require JS, not tightly coupled to Bootstrap, etc).

Anyways I'm closing as it's not worthy our time

simbus82 · 2023-10-26T12:32:52Z

More people have moaned about this because they don't like the design which should be fixed in the other PR than because they actually want a dark mode browser with a bright site.

This is your personal thinking. Facts are different.

wilsonge · 2023-10-26T20:18:40Z

Actually it’s based off the direct feedback that we had in mattermost from a dozen people during the betas and RCs. What’s your facts based on?

simbus82 · 2023-10-26T21:01:52Z

Actually it’s based off the direct feedback that we had in mattermost from a dozen people during the betas and RCs. What’s your facts based on?

Some billions of people 😅
#42134 (comment)

Only a small bunch of people. It's not good ignoring what large public in the world are using.

Introduce the switches

8c0aa44

Signed-off-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev>

dgrammatiko requested a review from chmst as a code owner October 23, 2023 17:34

joomla-cms-bot added Language Change This is for Translators PR-5.0-dev labels Oct 23, 2023

dgrammatiko mentioned this pull request Oct 23, 2023

There needs to be an option to disable dark mode in J5 Atum #42134

Closed

meh

ec2353f

Signed-off-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev>

Switch to tristate

947caa2

Signed-off-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev>

richard67 reviewed Oct 23, 2023

View reviewed changes

administrator/language/en-GB/com_users.ini Outdated Show resolved Hide resolved

Update administrator/language/en-GB/com_users.ini

58dd55c

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

richard67 reviewed Oct 23, 2023

View reviewed changes

administrator/language/en-GB/com_users.ini Outdated Show resolved Hide resolved

Update administrator/language/en-GB/com_users.ini

9266723

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

richard67 reviewed Oct 23, 2023

View reviewed changes

richard67 reviewed Oct 24, 2023

View reviewed changes

dgrammatiko closed this Oct 24, 2023

dgrammatiko deleted the 5.0-dev-tristate branch October 24, 2023 16:22

Fedik mentioned this pull request Oct 26, 2023

[5.x] Template color scheme configuration #42221

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.0] Allow OS color scheme override #42209

[5.0] Allow OS color scheme override #42209

dgrammatiko commented Oct 23, 2023 •

edited

richard67 commented Oct 23, 2023

dgrammatiko commented Oct 23, 2023

richard67 commented Oct 23, 2023

dgrammatiko commented Oct 23, 2023

brianteeman commented Oct 23, 2023

dgrammatiko commented Oct 23, 2023

richard67 commented Oct 23, 2023

wilsonge commented Oct 23, 2023 •

edited

dgrammatiko commented Oct 23, 2023

HLeithner commented Oct 23, 2023

dgrammatiko commented Oct 23, 2023

richard67 Oct 23, 2023

richard67 Oct 24, 2023

dgrammatiko Oct 24, 2023

richard67 Oct 24, 2023

bembelimen commented Oct 24, 2023

dgrammatiko commented Oct 24, 2023

simbus82 commented Oct 26, 2023

wilsonge commented Oct 26, 2023

simbus82 commented Oct 26, 2023 •

edited

[5.0] Allow OS color scheme override #42209

[5.0] Allow OS color scheme override #42209

Conversation

dgrammatiko commented Oct 23, 2023 • edited

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Link to documentations

richard67 commented Oct 23, 2023

dgrammatiko commented Oct 23, 2023

richard67 commented Oct 23, 2023

dgrammatiko commented Oct 23, 2023

brianteeman commented Oct 23, 2023

dgrammatiko commented Oct 23, 2023

richard67 commented Oct 23, 2023

wilsonge commented Oct 23, 2023 • edited

dgrammatiko commented Oct 23, 2023

HLeithner commented Oct 23, 2023

dgrammatiko commented Oct 23, 2023

richard67 Oct 23, 2023

Choose a reason for hiding this comment

richard67 Oct 24, 2023

Choose a reason for hiding this comment

dgrammatiko Oct 24, 2023

Choose a reason for hiding this comment

richard67 Oct 24, 2023

Choose a reason for hiding this comment

bembelimen commented Oct 24, 2023

dgrammatiko commented Oct 24, 2023

simbus82 commented Oct 26, 2023

wilsonge commented Oct 26, 2023

simbus82 commented Oct 26, 2023 • edited

dgrammatiko commented Oct 23, 2023 •

edited

wilsonge commented Oct 23, 2023 •

edited

simbus82 commented Oct 26, 2023 •

edited