Dark Theme Support: Allow for theme modifications #28233
Conversation
|
Size Change: +404 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
| if ( brightness > 100 ) { | ||
| // Is 100 the right number? | ||
| const body = ownerDocument.getElementsByTagName( 'body' )[ 0 ]; | ||
| // If we only remove it rather than adding it, then if themes don't declare support this will have no impact |
There was a problem hiding this comment.
That’s the only part that doesn’t sit all the way. I guess it would be a surprise to change, but it handling a “non dark mode theme with a dark background” I thought would be handy, and then adding that support wouldn't be necessary anywhere and could be depreciated.
There was a problem hiding this comment.
Thoughts outlined below suggesting delay of removal of the support make sense to me.
|
Fast work, impressive, and great PR. The editor needs to show dark UI (borders, focus styles, placeholder text) on light backgrounds, and light UI on dark backgrounds. The support you refer to is a boolean true/false choice, either your theme is one or the other. And specifically, the property, If I understand this PR correctly, this PR adds the If so, excellent! It does open a few questions:
3 is perhaps a bridge to cross when we get to it. This is, after all, a rabbit hole. But in principle, I'm a big fan of this, excited to land this. |
packages/block-editor/src/components/dark-editor-style/index.js
Outdated
Show resolved
Hide resolved
I believe it will yes
This won't work. We could detect both potentially.
No not quite...
They work together. Declaring support for dark theme will add the class, and this will remove the class if it's already present and the background is actually light. That means this will only work in themes that declare support. In some ways it makes more sense to do what @pbking suggests and add/remove the class based on the background, which means we don't need to declare theme support at all. This would be better for most people but there's more risk of causing issues with edge cases we didn't consider... |
We can cross that bridge when we get there.
Excellent, excellent thoughts. Since the support flag is already there, it seems to make a lot of sense to keep it for now and behave as you outline, and then we can remove it once it's been out there, tested a bit. What do you think? |
I agree |
There was a problem hiding this comment.
Awesome work. Gave this a quick spin in my own theme, which by default has a white background and black text.
Added add_theme_support( 'dark-editor-style' ); to my functions.php, and this support flag is now repurposed to say "be aware that the theme can have a black background".
The new color detection correctly detected that the background was light:
And when I then changed the colors, it correctly detected the background is dark:
Detecting this simply applies the is-dark-theme body class:
I even tried to be evil and feed it a background color of exactly mid-gray: rgb(127, 127, 127) — and found it interesting (but correct) to treat that as a dark theme:
Let's try this, and be mindful of any reports. And let's remember to potentially follow up with a PR that deprecates the add_theme_support( 'dark-editor-style' ); at some point in the future, if this continues to work as well as it seems to.
|
There was a failing test. I restarted to see if fortune favors us this time. |
packages/block-editor/src/components/dark-editor-style/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/dark-editor-style/index.js
Outdated
Show resolved
Hide resolved
| @@ -173,6 +174,7 @@ function Layout( { styles } ) { | |||
|
|
|||
| useDrop( ref ); | |||
| useEditorStyles( ref, styles ); | |||
| DarkEditorStyle( ref ); | |||
There was a problem hiding this comment.
Using it here means it only applies to edit-post. Should we do the same in all the other pages edit-widgets edit-navigation and edit-site?
There was a problem hiding this comment.
Probably edit-site, good catch. The other two, good question. @shaunandrews ?
There was a problem hiding this comment.
Both the widgets and navigation editors are more abstract representations of content, and I don't think they'll need to support this.
| useEffect( () => { | ||
| const { ownerDocument } = ref.current; | ||
| const editorStylesWrapper = ownerDocument.getElementsByClassName( | ||
| 'editor-styles-wrapper' |
There was a problem hiding this comment.
I don't like that it assumes the existence of a className. That said this className is indeed the one responsible for applying editor styles, so these things might be good together a EditorStylesWrapper component maybe?
There was a problem hiding this comment.
By looking at the code of the VisualEditor component and the number of hooks applied there, it seems that there's a clear node for some kind of reusable component for the block editor wrapper. But it something we can implement separately.
scruffian
force-pushed
the
update/dark-editor-style
branch
from
81a6fa2
to
56dd01e
Compare
|
@scruffian pushed a small refactor to use a callback ref, I didn't test properly with dark themes but the logic is the same I think. |
packages/block-editor/src/components/dark-editor-style/index.js
Outdated
Show resolved
Hide resolved
| @@ -57,13 +63,18 @@ export default function VisualEditor() { | |||
| useClipboardHandler( ref ); | |||
| useTypingObserver( ref ); | |||
| useCanvasClickRedirect( ref ); | |||
| const darkEditorRef = useDarkEditorStyle(); | |||
| const mergedRefs = useCallback( mergeRefs( [ ref, darkEditorRef ] ), [ | |||
There was a problem hiding this comment.
I believe we might have a hook in the works to replace this with useMergeRefs. Is this ready yet @ellatrix ?
| @@ -57,13 +63,18 @@ export default function VisualEditor() { | |||
| useClipboardHandler( ref ); | |||
| useTypingObserver( ref ); | |||
| useCanvasClickRedirect( ref ); | |||
There was a problem hiding this comment.
It seems like there's a number of hooks above that can be reworked as hooks that return "refs" instead of receiving refs (outside the scope of this PR)
scruffian
force-pushed
the
update/dark-editor-style
branch
from
9db58c9
to
97cfcef
Compare
youknowriad
force-pushed
the
update/dark-editor-style
branch
from
6357559
to
9db58c9
Compare
|
I believe this is now ready to be merged once the tests pass? |
scruffian
force-pushed
the
update/dark-editor-style
branch
from
dbb7143
to
d1ddd38
Compare
|
I keep rebasing but the tests keep failing. Any idea why? |
Co-authored-by: Ari Stathopoulos <aristath@gmail.com>
scruffian
force-pushed
the
update/dark-editor-style
branch
from
d1ddd38
to
e037fcd
Compare
It doesn't look like the failing tests are related to this PR... 🤔 |
How? |
|
I'm not sure the failing test is unrelated. At least, in master it seems to pass? and we did touch the edit-widgets package. |
Yep, turns out it must be caused by the PR. re-running the tests fails at the same tests so it's not a fluke.
Clicking "details" next to the failing test brings up the failing test's details, and there's a "Re-run jobs" button on the top. I don't know if it only shows if you have certain permissions or not, but it's there for me. |
|
I'm going to fix this. |
|
Thanks for fixing. |
…eader. With the changes to dark theme support in WordPress/gutenberg#28233 to check the real background color of the theme, this no longer serves any purpose. Follow-up to [44133]. Props scruffian, sabernhardt. Fixes #52385. git-svn-id: https://develop.svn.wordpress.org/trunk@50142 602fd350-edb4-49c9-b593-d223f7449a82
…eader. With the changes to dark theme support in WordPress/gutenberg#28233 to check the real background color of the theme, this no longer serves any purpose. Follow-up to [44133]. Props scruffian, sabernhardt. Fixes #52385. git-svn-id: https://develop.svn.wordpress.org/trunk@50142 602fd350-edb4-49c9-b593-d223f7449a82
…eader. With the changes to dark theme support in WordPress/gutenberg#28233 to check the real background color of the theme, this no longer serves any purpose. Follow-up to [44133]. Props scruffian, sabernhardt. Fixes #52385. Built from https://develop.svn.wordpress.org/trunk@50142 git-svn-id: http://core.svn.wordpress.org/trunk@49821 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…eader. With the changes to dark theme support in WordPress/gutenberg#28233 to check the real background color of the theme, this no longer serves any purpose. Follow-up to [44133]. Props scruffian, sabernhardt. Fixes #52385. Built from https://develop.svn.wordpress.org/trunk@50142 git-svn-id: https://core.svn.wordpress.org/trunk@49821 1a063a9b-81f0-0310-95a4-ce76da25c4cd
| const mergedRefs = useCallback( mergeRefs( [ ref, editorStylesRef ] ), [ | ||
| ref, | ||
| editorStylesRef, | ||
| ] ); |
There was a problem hiding this comment.
When does styles change? How can I trigger a change?
@youknowriad We discussed this a few times that adding dependencies here will give problems when passing new functions as a callback ref.
There was a problem hiding this comment.
Ah I see your comment now #28233 (comment)
I'll rebase #27768 in a bit and see what is missing there.
youknowriad
removed
the
Needs Dev Note
Description
Dark Theme support assumes that at theme's background is always dark, but this may not always be the case. Users can modify the theme background, and some themes background will be different if the system is in dark mode.
If a theme opts in to dark editor support then we add the is-dark-theme class to the body. This PR checks the real background color of the theme, and if its not dark then it removes this class.
Fixes #28200
How has this been tested?
Using Spearhead, which is a theme with dark mode
Screenshots
Light mode
Dark mode
Types of changes
Checklist: