Skip to content

Conversation

@wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Nov 9, 2023

Pull Request for Issue #42148 and #41801 .

Summary of Changes

Supports dark mode detection for both tinymce and codemirror. Unlike the template which will actually flick over immediately once the OS is changed this will not change until the page is next reloaded. For codemirror this may be possible in the future as outlined in the todo in the code comment. But at the moment I don't believe this is possible for tinymce.

I've reused the same data attribute that @Fedik is suggesting in #42221 (but I still disagree with the concept of a toggle in that PR - but this doesn't block that or alter that PR in any way). But the data attribute will be required on the body element to support this

Testing Instructions

Open the editor in both light and dark modes. Before it would always open in light mode. Now it will match whatever the template is using on page load.

Note that casseopia which doesn't support automatic dark mode remains having editors only loaded in light mode.

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: Note dark mode support Manual#206

  • [] No documentation changes for manual.joomla.org needed

@wilsonge wilsonge requested a review from chmst as a code owner November 9, 2023 09:36
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.0-dev labels Nov 9, 2023
@wilsonge wilsonge changed the title Add basic support for conditional dark mode themes in our editors [5.0] Add basic support for conditional dark mode themes in our editors Nov 9, 2023
@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 0069901

awesome!!!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42322.

@MacJoom
Copy link
Contributor

MacJoom commented Nov 10, 2023

I have tested this item ✅ successfully on b8eb1b7

Tested on Firefox with a dark mode toggle extension


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42322.

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on b8eb1b7


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42322.

@Quy
Copy link
Contributor

Quy commented Nov 10, 2023

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42322.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 10, 2023
@bembelimen
Copy link
Contributor

Not sure if this is the way forward, when we want to have a color switch soon based on a data attribute with a different name? (Although the overall function looks good)

@Fedik
Copy link
Member

Fedik commented Nov 15, 2023

It will be an adddittional atribute with the active color scheme name.
We just will be need to update this code to account it.

And data-color-scheme-os will stay, to indicate that the template will do switch automatically based on OS state.

@Quy Quy added the Feature label Nov 15, 2023
@Quy Quy mentioned this pull request Nov 16, 2023
@wilsonge
Copy link
Contributor Author

As @Fedik says and as I put in the description this uses the same data attribute that he's using in his pull request to implement the switcher. So I don't understand why this is incompatible with adding the switcher even if I disagree with it.

@bembelimen bembelimen merged commit 84585cc into joomla:5.0-dev Nov 18, 2023
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 18, 2023
@bembelimen
Copy link
Contributor

Thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants