-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Introduce distraction free mode #41740
Conversation
Size Change: +2.22 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
This looks like a good start to me. Since it's one part of some other "interface personalisation" ideas, maybe we should open a tracking issue that encapsulates everything? In terms of this PR specifically, I don't think that we necessarily need to disable other view options when the interface is hidden. It doesn't seem harmful to allow folks to toggle the top toolbar option for example. It would also be good if the top toolbar remained visible when the ellipsis menu is open. Edit: It would be good to restore the animations from #38928 (comment) as well, and refine. The notion of elements moving in and out of the viewport (rather than simply fading off the canvas) will feel more cohesive when we consider other views like the zoomed out one, and browse mode. |
I think it would be good to land this as an infrastructure base to build on. In the exploration PR there was a lot of back and forth to this point and even now the size of the change is considerable. So I wonder what is the minimum lacking here to land this and then start refining? |
I'd like to check this out for at least basic A11Y. I'll be around shortly. |
3ead5c4
to
548cda1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
f847053
to
8a09c31
Compare
This comment was marked as resolved.
This comment was marked as resolved.
af1f531
to
353c839
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Use the store directly instead. Co-authored-by: Daniel Richards <[email protected]>
Co-authored-by: Ben Dwyer <[email protected]>
I have merged this but it is not prime time ready yet. The PR:
It still requires a few updates to be at a level on par with other Gutenberg UX:
Worst case scenario this could be reverted to an experiment. |
Also, since the menu option is a checkbox, the menu should not close after enabling from the Options menu. It is considered bad UX to have one control in a menu act differently than all the others. I'm not really interested as to why this couldn't get done as long as it gets done in the future. Thanks. |
It's a very nice feature, well done on getting it to this stage. Another thing around the options menu (and also other dropdowns for consistency), It might be worth looking at keeping the top bar visible when focus is in the options menu dropdown, as the way it animates can lead to users clicking on the wrong thing: Kapture.2022-10-11.at.11.14.49.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work here, I left some remarks
packages/block-editor/src/components/block-list/use-in-between-inserter.js
Show resolved
Hide resolved
@@ -23,13 +24,12 @@ import MainDashboardButton from './main-dashboard-button'; | |||
import { store as editPostStore } from '../../store'; | |||
import TemplateTitle from './template-title'; | |||
|
|||
function Header( { setEntitiesSavedStatesCallback } ) { | |||
function Header( { setEntitiesSavedStatesCallback, isDistractionFree } ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a new prop instead of just using useSelect to retrieve it?
@@ -195,10 +201,14 @@ function wrapperSelector( select ) { | |||
?.__experimentalCaptureToolbars | |||
); | |||
|
|||
const settings = getSettings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSettings called here doesn't cause a re-render if the settings change, it should probably be called inside useSelect instead to avoid bugs.
@@ -36,7 +36,7 @@ const MoreMenu = ( { showIconLabels } ) => { | |||
scope="core/edit-post" | |||
/> | |||
) } | |||
<WritingMenu /> | |||
<WritingMenu onClose={ onClose } /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this change related to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch up review! 🙏🏻 This change - which I'll probably undo, idk - is to close the writing menu when entering this mode, because the top toolbar hides and this menu remains orphan in the UI.
@@ -20,6 +20,8 @@ export default function PreferenceToggleMenuItem( { | |||
messageActivated, | |||
messageDeactivated, | |||
shortcut, | |||
toggleHandler = () => null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe onToggle is a better name here?
<PreferenceToggleMenuItem | ||
scope="core/edit-post" | ||
name="distractionFree" | ||
toggleHandler={ toggleDistractionFree } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this handler at all? I thought the PreferenceToggleMenuItem
component already toggled the preference automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we need other stuff to happen (e.g. close sidebars) not only to toggle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why is this different than the other preference toggles above? the sidebars should close when the preference is set anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I did not alter the rendering of everything distraction free should close so that they render conditionally on the preference. Instead I created this cleanup handler. To have:
- the top toolbar disappear
- the inserter close
- the list view close
- the general sidebar close
- the 1st block focused
... when this preference is toggled on I'd have to pepper with checks for the isDistractionFree
in all these places (the last one would probably still need a special handler).
I think the answer to:
why is this different than the other preference toggles above?
is that distraction free has effects across the UI along the lines of availability not only behavior.
Hi @talldan and @alexstine I'll be looking into solving the problem described by Alex via the idea from Dan, to keep the header toolbar "visible" when the focus is in the writing menu. If I can do that, then I don't need to close the writing menu, hence solve Alex's problem too. |
The gist is this:
I have tried to solve this but have not had any success so far with convincing motion to kick in properly without rerendering the whole header component. I'll keep trying. PS I know @alexstine said " I'm not really interested as to why this couldn't get done as long as it gets done in the future" - but this is the future and I can't do it :))) there has to be a way, so far I am thinking of just trying with CSS again but framer motion does the easing so much better. |
@draganescu As a hot fix, can you simply add some type of |
How come, in the code we manually focus the 1st block in the editor specifically to not lose focus? Opening the more menu dropdown and focus one specific menu item after the header is remounted seems quite convoluted. There has to be a way to avoid the need to re-mounting. |
@draganescu To replicate:
It seems this only happens when distraction free is disabled so my mistake if this wasn't clear. Thanks. |
@draganescu @alexstine I've put a fix together for the focus loss issue in this PR - #51627. It seems to resolve the issue in the majority of cases, though I am still seeing a focus loss on the rare occasion. |
I've updated and approved that PR 👏🏻 |
Awesome, having a look now. 👍 |
What?
This PR is the result of the exploration in #38928 to introduce a
distraction free mode. This mode build on top of the reduce interface
preference, amplifying its effect.
Why?
To enhance content production in WordPress. Text based content is one of
the most important forms of content. While the site and page building
enhancements make multi media content production much easier, through the
visual tools offered by the block editor, text content production can
sometimes feel impaired.
A distraction free mode (the term became somewhat of an industry standard)
aims to remove visual clutter when the focus is on text based content
production.
How?
The PR builds on the reduce interface prefference. Until now the
preference, when toggled on, would only hide some of the toolbar icons and
a few other non-essential UI items. With this PR the prefference will have
a more drastic effect, turning the UI into a blank page:
area
Known issues
distraction free mode is on and the
ESC
key is pressedbreak some things
hasReducedUi
props need to be refactored intoisDistractionFree
Testing Instructions
Screenshots or screencast
distraction-free-motion.mp4