Site Editor: Add 'area' selection to convert to template part flow. #30395
Conversation
Addison-Stavlo
added
the
[Feature] Site Editor
|
cc @jameskoster - how do you feel about the styles and interaction at this point? |
|
Size Change: +917 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
|
Everything works as expected :) I made a couple of adjustments to the design to better match other similar UI elements. Mainly using the checkmark to indicate selection rather than the background color:
|
Interesting, where else do we use this checkmark currently? (still waking up / drawing a blank on this 😆 )
This is what the button styles currently do by default (the opacity drop):
It feels a bit ugly in this case to me. I think the checkbox or dark background both work better. |
Oh, I just realized you added those changes to this PR. 😆 Thanks a bunch! |
|
Here is a pic of me hovering over the selected button:
I assume we want to get rid of the hover effect on the selected item (no change to blue)? |
It's a slightly different application to this, but the checkmark appears in the options here:
Ah yes, good spot. |
Ah right! Thanks for pointing it out. I have seen them plenty of times in the past but they don't stick in my memory. Thats probably a good sign that they do their job well without forcing me to think about them 😆 |
|
Updated a bit to see if we could get better a11y navigation that will scale with more options in the future. I kind of got in a rabbit-hole with VoiceOver, things seem to work pretty well except my one complaint now is it doesn't announce when I'm in the "Area" selection region, it just immediately starts talking about the "General" option (while I think a voiceover user would be asking "what is this an option for?"). 🤔 |
|
I'm not too familiar with voice over functionality, do you think some helper text beneath the area options could help there? Or even simpler – a (slightly) more descriptive label? |
Im not super familiar with it as well. But it seems like the label and/or helper text I tried aren't read when we navigate into that selection area. (For the 'name' input above it is, and for our similar dropdown/select input for area in the block inspector for the TP block it is.. but I guess this interaction is a bit different for it). It could be a quirk of the specific voiceover software as well. All that said though, I think it is still usable and it may not be a huge deal (although it would be nice to fix that tiny bit). It does announce that it is a radio type item selection, whether or not the item is selected, and reads the area description for the first item which does add a lot of context. |
jameskoster
added
the
[Focus] Accessibility (a11y)
packages/edit-site/src/components/template-part-converter/convert-to-template-part.js
Outdated
Show resolved
Hide resolved
I can reproduce this as well. I'm not entirely certain what's causing it, but I started digging into the wrapper BaseControl element we're using. A few things I noticed:
|
The one element would be the
oh good catch! I missed that same id was applied to the inner element. I was thinking that id was set on the bascontrol and the for on its corresponding |
Ahhh that makes sense. To clarify, did you add the
I think it still might be an issue because I'm not sure if the Edit: "Some elements, not all of them form-associated, are categorized as labelable elements. These are elements that can be associated with a label element. I don't think the |
|
🤔 That makes sense, but im not sure what to do about it. We could use the 'as' prop if there was a form associated element that would work for this case. This also seems to be a problem with some pre-existing components as well (such as RadioGroup/Radio), as the RadioGroup also renders as a div. Note - I could be making an issue out of nothing. Im not super familiar with using voiceover and after testing out other examples (like radio inputs in w3 etc.) the voiceover seems to read about the same as here. Its probably best not to invest too much time into this issue before we get some feedback from a11y. |
Addison-Stavlo
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
Addison-Stavlo
requested review from
vindl,
jeyip,
mattwiebe,
Copons,
david-szabo97 and
creativecoder
|
Besides that a11y ❓ in so much discussion above, this should be ready for review. |
|
Thanks for taking a look @ntsekouras ! Updated per your suggestions. |
There was a problem hiding this comment.
Nice work @Addison-Stavlo 💯
With happy CI (some valid php linting issues right now) let's 🚢 !
Addison-Stavlo
force-pushed
the
add/convert-to-template-part-area-selection
branch
from
dd74d5f to
e865df8
Compare
|
Testing with WordPress 5.7 Selecting an image in the Index template. Clicking "Make template part"
I see the screen with three options. General, Header and Footer. It uses the text: |
|
Thanks for taking a look @paaljoachim!
We can definitely update the descriptions if needed. But I think the idea in the future is to actually support a "Sidebar" area in addition to header/footer/general. Its a bit more complicated than the other cases and we dont yet have the in-editor tools to support it, but I would assume once we get to developing sidebar support it would be its own item in this list? cc @jameskoster |
|
We can perhaps simplify the description a bit by leaving out the example.
Is that better? |
|
Yeah! That fits in nicely with a general template parts area. |
|
#30811 - heres a PR to update the description. Feel free to update the copy if needed. |
priethor
changed the title
Description
Attempts to resolve #29222 - by adding selection for 'area' in this modal per design suggestion #29222 (comment) (update to #30395 (comment))
How has this been tested?
To Test:
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).