Skip to content
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

Add element selector to template-part block #27101

Merged
merged 6 commits into from
Dec 7, 2020

Conversation

aristath
Copy link
Member

Description

Fixes #27092

This PR adds a SelectControl to the template-part block, to allow selecting what the template-part's wrapper element should be.

How has this been tested?

Tested in an FSE theme in the site editor:

  • Selected the theme's template parts, confirmed that I was able to change them to their proper semantically-correct values
  • Saved & reloaded the editor, confirmed that values persist
  • Removed the patch and confirmed nothing breaks (backwards-compatible)
  • Confirmed that the selected values are properly mirrored on the frontend.

Screenshots

Peek 2020-11-19 15-42

Types of changes

  • Added a SelectControl with values that are applicable to template-parts

Most of the changes in the diff are indentation changes because adding the control required nesting the elements inside a wrapper.

There are no breaking changes. We already had a tagName property in the block, but it required manual intervention in the HTML - which was impossible to do since the site-editor doesn't have a code view.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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.

@github-actions
Copy link

github-actions bot commented Nov 19, 2020

Size Change: +130 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-library/index.js 149 kB +130 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 11.2 kB 0 B
build/block-editor/style.css 11.2 kB 0 B
build/block-library/editor-rtl.css 9.07 kB 0 B
build/block-library/editor.css 9.07 kB 0 B
build/block-library/style-rtl.css 8.34 kB 0 B
build/block-library/style.css 8.34 kB 0 B
build/block-library/theme-rtl.css 789 B 0 B
build/block-library/theme.css 790 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/core-data/index.js 15.2 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.98 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.49 kB 0 B
build/edit-post/style.css 6.47 kB 0 B
build/edit-site/index.js 24.5 kB 0 B
build/edit-site/style-rtl.css 3.91 kB 0 B
build/edit-site/style.css 3.91 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 43.6 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.63 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.74 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.93 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/notices/index.js 1.82 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.4 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 2.84 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@gziolo gziolo added the [Block] Template Part Affects the Template Parts Block label Nov 19, 2020
@gziolo gziolo self-requested a review November 20, 2020 14:16
@gziolo
Copy link
Member

gziolo commented Nov 20, 2020

For a better context, we tried something similar with the Group block that has tagName attributes. We explored in #20218 making it possible to pick different tag names through block variations exposed in the inserter but we didn't proceed with the solution eventually, reverted code for reference:

de6b2ed

Screen Shot 2020-02-13 at 17 49 34

@carolinan
Copy link
Contributor

So far I have used block variations for template-parts as a way to speed up the development when I test FSE themes and for me it has worked really well.

Despite this, I think the drop down is a better solution:

  • It is easy to change the element.
  • The variations can be confusing for non developers, especially if a template part with the variation name "header" can be confused with selectable template parts for site headers.

I suggest the following changes:
Place the semantics under the Advanced section to avoid confusion for users who are not familiar with HTML.

Remove the word "Wrapping" and only keep "HTML element"

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise, it's good to go after addressing the nitpick about the default value in the TemplatePartEdit component prop.

Moving forward, we will need a strategy to introduce unified handling for blocks to act as wrapping elements:

  • Group
  • Post Content
  • Query
  • Reusable Block
  • Template Part

or blocks that can have different wrapping tag elements depending on function:

  • Heading
  • List

It would be great to hear from @mtias or @jasmussen on the above before we commit to anything.

(The failing e2e test was improved after the last commit pushed from this branch, rebase should resolve it)

packages/block-library/src/template-part/edit/index.js Outdated Show resolved Hide resolved
@jasmussen
Copy link
Contributor

Instead of adding a new panel called "Semantics", can we add this to the "Advanced" section?

@aristath
Copy link
Member Author

aristath commented Dec 7, 2020

Instead of adding a new panel called "Semantics", can we add this to the "Advanced" section?

Done in 5f2fa20 👍

@aristath
Copy link
Member Author

aristath commented Dec 7, 2020

Remove the word "Wrapping" and only keep "HTML element"

Done in 1b0c37d 👍

@jasmussen
Copy link
Contributor

I think there's an opportunity to further refine this with some of the component work and systems that are outlined in #27331, but so long as this is in the advanced section and is collapsed by default, seems fine to move forward with this one as a feature addition. Nice work.

@gziolo
Copy link
Member

gziolo commented Dec 7, 2020

I have just tested it and it work according to the latest suggestion from @jasmussen. Let's get this in 🎉

@aristath
Copy link
Member Author

aristath commented Dec 7, 2020

Thank you @gziolo for your testing, I'll go ahead and merge it 👍

@aristath aristath merged commit 8ccfc73 into master Dec 7, 2020
@aristath aristath deleted the add/template-part-semantics branch December 7, 2020 12:48
@github-actions github-actions bot added this to the Gutenberg 9.6 milestone Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Template Part Affects the Template Parts Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow selecting HTML element for the template-part wrapper
4 participants