-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix site editor region navigation #36709
Conversation
Size Change: +289 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
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.
Thank you!
[ templateType ] | ||
); | ||
|
||
const itemsListLabel = postType?.labels?.items_list; |
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.
I think it would be undefined
when initially rendered, that would make the labels below having something like "undefined - Header"
in their strings. I wonder if we should just render null
if postType
isn't available yet?
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.
I think it's preloaded. Didn't realise it could still be null
.
Wouldn't we also have this issue elsewhere in the UI?
For the sake of this PR, I'll make it fallback to the normal labels (which are just 'Header', 'Content').
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.
Yeah probably. I think it should not be null
if it's preloaded. That's a separate issue though.
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.
LGTM 👍
* Implement shortcuts and labels * Use correct key for body * Fix focusing navigation sidebar when navigating regions * Only use detailed region labels when post type has loaded
Description
Fixes a few issues with region navigation in the site editor:
tabIndex=-1
which meant it couldn't be focused when navigating regionsThe outline styles for the regions could still be improved in the site editor. I haven't tackled that in this PR, because I need to work on some other important tasks.
How has this been tested?
Testing steps depend on which OS you're using. I think on a mac it's recommended to use ctrl + ` because the alternative shortcut (ctrl+alt+n) seems to toggle a setting in voiceover that you really don't want to toggle. Also best to test in Safari on a mac.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.js
files for terms that need renaming or removal).