Small follow-ups to the distraction free mode PR #45151
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
the
[Type] Code Quality
youknowriad
commented
Oct 20, 2022
| @@ -38,6 +39,8 @@ function Header( { setEntitiesSavedStatesCallback, isDistractionFree } ) { | |||
| isSaving: select( editPostStore ).isSavingMetaBoxes(), | |||
| showIconLabels: | |||
| select( editPostStore ).isFeatureActive( 'showIconLabels' ), | |||
| isDistractionFree: | |||
There was a problem hiding this comment.
No need for prop passing from parent to children since we can access this here. The pro here is that the "Header" component becomes more reusable and if we remove the isDistractionFree selector from the parent it would result in a small perf optimization.
youknowriad
commented
Oct 20, 2022
| closeGeneralSidebar(); | ||
| if ( ! isDistractionFree && !! blocks.length ) { | ||
| selectBlock( blocks[ 0 ].clientId ); | ||
| } |
There was a problem hiding this comment.
I think we can probably get rid of all of these if we just not render them in distraction free mode (remove the inserter, remove list view, remove the sidebar) that way if you switch back to regular mode, they restore to their previous state.
I didn't want to make that change here though.
There was a problem hiding this comment.
Yea commented to the original PR that this is the second option to this handler. It's a better way but it peppers the code with a lot of extra isDistractionFree checks and my goal is to have the least impact initially.
youknowriad
commented
Oct 20, 2022
|
|
||
| useEffect( () => { | ||
| setDistractionFree( isDistractionFree ); | ||
| }, [ isDistractionFree ] ); |
There was a problem hiding this comment.
AFAIK, this was useless local state.
There was a problem hiding this comment.
These bits of code are surprising. I wouldn't have added it for no reason :D 🤷🏻 Probably didnt remove it on iteration X when it wasn't needed anymore 🙇🏻
There was a problem hiding this comment.
Yeah, sometimes it's hard to keep track of all the necessary/unnecessary bits as we iterate over feedback.
|
Size Change: +2 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
youknowriad
commented
Oct 20, 2022
| setIsInserterOpened( false ); | ||
| setIsListViewOpened( false ); | ||
| closeGeneralSidebar(); | ||
| if ( ! isDistractionFree && !! blocks.length ) { |
There was a problem hiding this comment.
This is a bug fix where toggling distraction free would fail if the post was empty
Mamaduka
approved these changes
Oct 20, 2022
Yeah, I noticed this when working on my blog post on keyboard shortcuts. Basically, we only catch keyboard shortcuts if the focus is within the "shortcut provider". So we'd have two options here:
I'd start with the latter personally since the focus loss itself is probably an a11y issue. |
That explains the "focus on mount" in the code example 😄 I like the second option. Maybe we can focus on the first tabbable element in the header area, after switching the mode. P.S. The code editor also has a similar issue - #42145. |
alexstine
approved these changes
Oct 21, 2022
There was a problem hiding this comment.
@youknowriad Do you have time to refactor this?
I am not super pleased that one menu option works differently then the other options in that list. Is there anyway to make the toolbar appear on hover/focus that way the options menu item can still receive focus without it looking super weird visually? Then once the user navigates away from it, it can disappear. That is how the toolbars work in full screen browser mode, not sure why it cannot happen here.
Other than that, LGTM.
Thanks.
To be honest I'm not entirely sure what you're talking about 😬 which toolbars/menu items... |
|
@youknowriad If you enable the distraction free mode from the options menu, the toolbar gets hidden and focus is shifted to the block list. This works differently then all the other options within the menu. For example, toggling full screen mode will check the box, but focus stays in the menu. It is an anti-pattern to have one item act differently in a menu of items that work exactly the same. Does that help? Thanks. |
|
@alexstine yes, that makes sense thank you and I agree. I'll see if I can improve that separately |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Just addressing some of my comments on this PR #41740 (review) (more explanations inline)
Testing Instructions