Interface: move EditorSkeleton to interface package #21335
Conversation
vindl
requested review from
jorgefilipecosta,
ockham,
noahtallen and
Addison-Stavlo
|
Size Change: +3.95 kB (0%) Total Size: 889 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This looks comprehensive, I gave the editors a smoke test and things still seem to be in good shape.
I'm really not sure what package this should live in, but interface seems reasonable.
What is the underlying reason for this migration? To move things usable by both block/site editors to a more neutral package?
Yes, and we didn't have the adequate package for that before so |
|
Makes sense to me. Do you think we need to note anything in the interface package Readme about these? They don't seem to have been listed in the block-editor one, but maybe it makes more sense as a more permanent home? (or maybe wait since they are both (note for anyone else reading: by 'both' I am also referring to #21334) |
This is likely going to be their permanent home so I'm not sure if they should still be prefixed as |
|
Might be worth changing the naming to be a bit more generic, so it’s not specifically named around “editor”? Also I think it might be expected that class names are updated to be prefixed with |
I agree with this but when we do that, we should also make the "region labels" editable maybe using a "labels" prop or something like that. |
|
I think (correct me if I'm wrong @jorgefilipecosta ) that the interface package is a "bundled" package that doesn't leak its API to Core. This means we can safely remove "__experimental" and we can still do breaking changes in the future if needed. |
I'm fine with that. @chrisvanpatten @youknowriad any suggestions for the alternative name? |
|
@vindl InterfaceSkeleton? Or just Skeleton? It's also probably worth coming up with an alternate name for the "publish" region. I'm not sure exactly what that corresponds to in the UI — would it be the publish panel in the normal editor? Perhaps something like "Skeleton Action" region or something? Obviously if the region names are configurable then the consuming package can choose an appropriate name. |
vindl
force-pushed
the
update/editor-skeleton-to-interface
branch
from
adef3ed
to
3b4dd23
Compare
|
Renamed to |
Yes, it does. I renamed it to
@youknowriad I added the option to pass the labels as props. I'm not sure if changing the region names themselves makes much sense given that their structure and nesting is hardcoded in the component. |
| sidebar: __( 'Editor settings' ), | ||
| actions: __( 'Editor publish' ), | ||
| footer: __( 'Editor footer' ), | ||
| }; |
There was a problem hiding this comment.
Do you think we should drop the "Editor" from the default labels?
There was a problem hiding this comment.
I also feel it works better for most UIs without "Editor" (like widgets)
There was a problem hiding this comment.
Yes, that sounds fine to me. 👍
There was a problem hiding this comment.
Updated in 2e1d036ae02a7f5eceed238059815bb05e750284.
There was a problem hiding this comment.
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.
Do you think we should drop the "Editor" from the default labels?
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:
For better accessibility, these elements must be properly labelled to briefly describe the purpose of the content in the region. For more details, see "ARIA landmarks" in the WAI-ARIA specification.
Will create a new issue.
vindl
force-pushed
the
update/editor-skeleton-to-interface
branch
from
2e1d036
to
9f64def
Compare
| @@ -61,6 +61,14 @@ function Editor( { settings: _settings } ) { | |||
| }; | |||
| }, [] ); | |||
|
|
|||
| const skeletonLabels = { | |||
There was a problem hiding this comment.
I wonder if we really need these now, as the default ones apply generically :)
There was a problem hiding this comment.
Pretty easy to change them later on if needed. I'm going to merge this to avoid more of the rebase hell. 😄
That is correct :) |
Description
Moves EditorSkeleton component from
block-editortointerfacepackage.How has this been tested?
Smoke test the post, site, and widgets editor and verify that they still work as expected.
Types of changes
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Checklist: