-
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
Interface: move EditorSkeleton to interface package #21335
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
export { default as ComplementaryArea } from './complementary-area'; | ||
export { default as FullscreenMode } from './fullscreen-mode'; | ||
export { default as InterfaceSkeleton } from './interface-skeleton'; | ||
export { default as PinnedItems } from './pinned-items'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,72 +24,84 @@ function useHTMLClass( className ) { | |
}, [ className ] ); | ||
} | ||
|
||
function EditorSkeleton( { | ||
function InterfaceSkeleton( { | ||
footer, | ||
header, | ||
sidebar, | ||
content, | ||
publish, | ||
actions, | ||
labels, | ||
className, | ||
} ) { | ||
useHTMLClass( 'block-editor-editor-skeleton__html-container' ); | ||
useHTMLClass( 'interface-interface-skeleton__html-container' ); | ||
|
||
const defaultLabels = { | ||
header: __( 'Header' ), | ||
body: __( 'Content' ), | ||
sidebar: __( 'Settings' ), | ||
actions: __( 'Publish' ), | ||
footer: __( 'Footer' ), | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think we should drop the "Editor" from the default labels? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also feel it works better for most UIs without "Editor" (like widgets) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that sounds fine to me. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in 2e1d036ae02a7f5eceed238059815bb05e750284. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this changes the labels for the widgets and more importantly the "Post editor". For me it's still good names and I like the fact that they are the same across pages. Let's keep them consistent for now and if we have feedback suggesting to go specific for some of these screens, we'll adapt. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When used to label the ARIA landmark regions in the context of the WP admin, these labels need to differentiate the Editor landmarks from the other landmarks in the WP UI. This is the reason why they used the word "Editor" at the beginning. For some history, see #7940, #7938, and #2380 Also, the ARIA landmarks labels need to describe the purpose of the content within the region. Labels like "Header", "Footer", and "Left Sidebar" (added in #20951) are arguably og any usefulness for assistive technology users. If these labels are going to be used also for other purposes, then we'll need to provide specific labels for the ARIA landmarks. Worth also noting the ARIA regions labels usage is still documented in the regions Readme:
Will create a new issue. |
||
|
||
const mergedLabels = { ...defaultLabels, ...labels }; | ||
|
||
return ( | ||
<div | ||
className={ classnames( | ||
className, | ||
'block-editor-editor-skeleton' | ||
'interface-interface-skeleton' | ||
) } | ||
> | ||
{ !! header && ( | ||
<div | ||
className="block-editor-editor-skeleton__header" | ||
className="interface-interface-skeleton__header" | ||
role="region" | ||
/* translators: accessibility text for the top bar landmark region. */ | ||
aria-label={ __( 'Editor top bar' ) } | ||
aria-label={ mergedLabels.header } | ||
tabIndex="-1" | ||
> | ||
{ header } | ||
</div> | ||
) } | ||
<div className="block-editor-editor-skeleton__body"> | ||
<div className="interface-interface-skeleton__body"> | ||
<div | ||
className="block-editor-editor-skeleton__content" | ||
className="interface-interface-skeleton__content" | ||
role="region" | ||
/* translators: accessibility text for the content landmark region. */ | ||
aria-label={ __( 'Editor content' ) } | ||
aria-label={ mergedLabels.body } | ||
tabIndex="-1" | ||
> | ||
{ content } | ||
</div> | ||
{ !! sidebar && ( | ||
<div | ||
className="block-editor-editor-skeleton__sidebar" | ||
className="interface-interface-skeleton__sidebar" | ||
role="region" | ||
/* translators: accessibility text for the settings landmark region. */ | ||
aria-label={ __( 'Editor settings' ) } | ||
aria-label={ mergedLabels.sidebar } | ||
tabIndex="-1" | ||
> | ||
{ sidebar } | ||
</div> | ||
) } | ||
{ !! publish && ( | ||
{ !! actions && ( | ||
<div | ||
className="block-editor-editor-skeleton__publish" | ||
className="interface-interface-skeleton__actions" | ||
role="region" | ||
/* translators: accessibility text for the publish landmark region. */ | ||
aria-label={ __( 'Editor publish' ) } | ||
aria-label={ mergedLabels.actions } | ||
tabIndex="-1" | ||
> | ||
{ publish } | ||
{ actions } | ||
</div> | ||
) } | ||
</div> | ||
{ !! footer && ( | ||
<div | ||
className="block-editor-editor-skeleton__footer" | ||
className="interface-interface-skeleton__footer" | ||
role="region" | ||
/* translators: accessibility text for the footer landmark region. */ | ||
aria-label={ __( 'Editor footer' ) } | ||
aria-label={ mergedLabels.footer } | ||
tabIndex="-1" | ||
> | ||
{ footer } | ||
|
@@ -99,4 +111,4 @@ function EditorSkeleton( { | |
); | ||
} | ||
|
||
export default navigateRegions( EditorSkeleton ); | ||
export default navigateRegions( InterfaceSkeleton ); |
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 wonder if we really need these now, as the default ones apply generically :)
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.
Pretty easy to change them later on if needed. I'm going to merge this to avoid more of the rebase hell.😄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.
Actually scratch that, I updated this in 68c5e26 after reading your comment here.