Template Part Block: Use buttons for area selection #30005

creativecoder · 2021-03-19T04:34:45Z

Description

Resolves #29295

How has this been tested?

Open the site editor
Select a template part block, such as the header (the list view is an easy way to do this).
Open the Advanced section of the sidebar options and choose the area.

Screenshots

Template Part Block > Advanced settings in sidebar

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

jameskoster · 2021-03-19T09:38:06Z

Looking good! :)

I updated the icon / description for the general area.

The width of these tooltips makes them a little awkward to read. Is it possible to wrap them?

creativecoder · 2021-03-19T19:32:07Z

I updated the icon / description for the general area.

Thank you!

The width of these tooltips makes them a little awkward to read. Is it possible to wrap them?

Yes! Now updated.

I'm a little unsure about the screen reader accessibility of using tooltips to describe the template part areas. I modified the Tooltip component to be able to receive an id and be the target of aria-describedby (declared on the buttons)--this seems to help when testing with VoiceOver, but I'd want a more experienced opinion before merging this.

david-szabo97 · 2021-03-22T09:03:22Z

packages/components/src/tooltip/index.js

+	position,
+	text,
+	shortcut,
+	...popoverProps


Hmmmm 🤔 This seems to be a rather big modification. I wonder if we should use an explicit popoverProps property instead of ... rest. We need to update the README too.

As a last thought, maybe we should move this to a separate PR?

david-szabo97 · 2021-03-22T09:06:40Z

It's working as expected. My only concern is the modification of the Tooltip component. The changes make sense but it feels it should be its own PR. Looking forward to others' thoughts.

Screen.Recording.2021-03-22.at.9.58.18.mov

jameskoster · 2021-03-22T12:09:43Z

+1 to moving the a11y discussion to a separate issue since it will effectively relate to all tooltip usage across the app.

david-szabo97 · 2021-03-24T10:07:38Z

@creativecoder Could you break this into multiple PRs?

We should remove the tooltip changes from this PR and create a separate PR for just the tooltip. Then we can create a follow-up PR to add the accessibility parts once we have the tooltip PR merged. You can use the tooltip PR as a base for the follow-up PR so we can show a use-case as well in the tooltip PR.

So basically we will have 3 PRs in the end:

This PR which introduces the buttons for area selection (without a11y in mind)
Tooltip PR which adds the new prop to the Tooltip component so we can passthrough props to the Popover component
A accessibility follow-up PR to the 1. one, using 2. as the base branch

creativecoder · 2021-03-25T03:53:25Z

Could you break this into multiple PRs?

Yes! Will work on that soon. I'd like to leave the className passthrough (Tooltip prop that's passed to the Popover component) so we can control the tooltip width. That seems less invasive, and therefore okay? I'll be sure to update the component readme, accordingly.

david-szabo97 · 2021-03-25T18:46:50Z

I'd like to leave the className passthrough (Tooltip prop that's passed to the Popover component) so we can control the tooltip width. That seems less invasive, and therefore okay?

I would be still concerned about the PR's responsibility. In my opinion, it's better to avoid making core changes in a mostly Site Editor related PR. Smaller PRs and single responsibility PRs are easier to revert and discuss.

Addison-Stavlo

Just some minor thoughts, its lookin pretty good so far. 😁

Addison-Stavlo · 2021-04-05T15:14:29Z

packages/block-library/src/template-part/edit/advanced-controls.js

-						value={ area }
-						onChange={ setArea }
-					/>
+					<BaseControl


I wonder if RadioGroup/Radio components would be able to fit well and be adaptable for this case? It seems pretty similar visually and behaviorally to what we are going for.

+1 for RadioGroup and Radio components. They seem to fit this particular situation.

Thanks for the idea, I will take a look.

Addison-Stavlo · 2021-04-05T15:15:03Z

packages/block-library/src/template-part/edit/advanced-controls.js

+		description: __(
+			'The Header template defines a page area that typically contains a title, logo, and main navigation.'
+		),
+		icon: header,
+		label: __( 'Header' ),
+		value: 'header',


Not something that needs changed in this PR, but we should centralize these descriptions/definitions so we don't need to re-define them in a handful of places. I'm trying that as part of #30395, so a follow up once these both land would be to update this to get that data from the core editor store.

jameskoster

Approving the design. If the width has to be fixed in a follow-up PR, so be it :D

david-szabo97 · 2021-04-30T09:49:00Z

👋 Would love to see this merged 😄 Could you rebase it? Happy to approve once tests are passing and rebased. We can create a follow-up PR for the a11y comment.

creativecoder · 2022-11-15T02:13:34Z

Closing in favor of #45764, which uses the newer ToggleGroupControl component.

Template Part Block: Use buttons for area selection

7160e77

creativecoder self-assigned this Mar 19, 2021

creativecoder added the [Block] Template Part Affects the Template Parts Block label Mar 19, 2021

creativecoder requested review from vindl, jeyip, mattwiebe, david-szabo97, Addison-Stavlo, Copons and jameskoster March 19, 2021 04:40

jameskoster mentioned this pull request Mar 19, 2021

Template part: Area selection UI #29295

Open

jameskoster added 2 commits March 19, 2021 09:36

Update general area icon and description

790d311

formatting

27f748a

Format template part area descriptions

fbc48c3

creativecoder marked this pull request as ready for review March 19, 2021 19:32

creativecoder requested review from ajitbohra and chrisvanpatten as code owners March 19, 2021 19:32

creativecoder changed the title ~~[WIP] Template Part Block: Use buttons for area selection~~ Template Part Block: Use buttons for area selection Mar 19, 2021

Removes duplicate prop

20e1060

david-szabo97 reviewed Mar 22, 2021

View reviewed changes

critterverse mentioned this pull request Mar 23, 2021

Fixed Position Header and Footer Template Parts #30121

Closed

Addison-Stavlo reviewed Apr 5, 2021

View reviewed changes

jameskoster approved these changes Apr 6, 2021

View reviewed changes

javierarce mentioned this pull request Apr 13, 2021

Save Panel - Add template descriptions. #29463

Closed

creativecoder mentioned this pull request Nov 15, 2022

Template Part Block: use icon for area selection (take 2) #45764

Closed

creativecoder closed this Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template Part Block: Use buttons for area selection #30005

Template Part Block: Use buttons for area selection #30005

creativecoder commented Mar 19, 2021 •

edited

Loading

jameskoster commented Mar 19, 2021

creativecoder commented Mar 19, 2021

david-szabo97 Mar 22, 2021

david-szabo97 commented Mar 22, 2021

jameskoster commented Mar 22, 2021

david-szabo97 commented Mar 24, 2021

creativecoder commented Mar 25, 2021

david-szabo97 commented Mar 25, 2021

Addison-Stavlo left a comment

Addison-Stavlo Apr 5, 2021 •

edited

Loading

david-szabo97 Apr 6, 2021

creativecoder Apr 7, 2021

Addison-Stavlo Apr 5, 2021

jameskoster left a comment

david-szabo97 commented Apr 30, 2021 •

edited

Loading

creativecoder commented Nov 15, 2022

Template Part Block: Use buttons for area selection #30005

Template Part Block: Use buttons for area selection #30005

Conversation

creativecoder commented Mar 19, 2021 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

jameskoster commented Mar 19, 2021

creativecoder commented Mar 19, 2021

david-szabo97 Mar 22, 2021

Choose a reason for hiding this comment

david-szabo97 commented Mar 22, 2021

jameskoster commented Mar 22, 2021

david-szabo97 commented Mar 24, 2021

creativecoder commented Mar 25, 2021

david-szabo97 commented Mar 25, 2021

Addison-Stavlo left a comment

Choose a reason for hiding this comment

Addison-Stavlo Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

david-szabo97 Apr 6, 2021

Choose a reason for hiding this comment

creativecoder Apr 7, 2021

Choose a reason for hiding this comment

Addison-Stavlo Apr 5, 2021

Choose a reason for hiding this comment

jameskoster left a comment

Choose a reason for hiding this comment

david-szabo97 commented Apr 30, 2021 • edited Loading

creativecoder commented Nov 15, 2022

creativecoder commented Mar 19, 2021 •

edited

Loading

Addison-Stavlo Apr 5, 2021 •

edited

Loading

david-szabo97 commented Apr 30, 2021 •

edited

Loading