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 a way to change template parts. #24990

Merged
merged 9 commits into from
Sep 9, 2020

Conversation

Addison-Stavlo
Copy link
Contributor

Description

This PR adds a button in the toolbar that will allow a user to 'Choose another' template part. This uses the same search/preview selection component as the Template Part Placeholder block (as well as refactors this for reuse outside of the placeholder itself.)

This should be a good starting point being able to switch template parts, and reusing the same search and preview component as the placeholder will allow us to maintain consistency and improve this in both places at the same time in future iterations.

The Icon is currently "update", and can obviously be changed if something more fitting is suggested (I looked at "search" and "tool" as well, but am open to other suggestions.)

How has this been tested?

On local environment. Tested both flows from existing template part as well as from the placeholder block.

Screenshots

change-template-part

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.
  • 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 Sep 1, 2020

Size Change: +298 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/a11y/index.js 1.14 kB +1 B
build/block-directory/index.js 8.5 kB -1 B
build/block-editor/index.js 128 kB +4 B (0%)
build/block-library/editor-rtl.css 8.68 kB +43 B (0%)
build/block-library/editor.css 8.68 kB +44 B (0%)
build/block-library/index.js 139 kB +255 B (0%)
build/blocks/index.js 47.7 kB +1 B
build/components/index.js 200 kB -61 B (0%)
build/data/index.js 8.55 kB +1 B
build/edit-post/index.js 305 kB +3 B (0%)
build/edit-site/index.js 19.4 kB +3 B (0%)
build/edit-widgets/index.js 12.1 kB +2 B (0%)
build/editor/index.js 45.6 kB +2 B (0%)
build/format-library/index.js 7.72 kB +2 B (0%)
build/redux-routine/index.js 2.85 kB -2 B (0%)
build/url/index.js 4.06 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.41 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/style-rtl.css 7.59 kB 0 B
build/block-library/style.css 7.58 kB 0 B
build/block-library/theme-rtl.css 754 B 0 B
build/block-library/theme.css 754 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.7 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/style-rtl.css 6.26 kB 0 B
build/edit-post/style.css 6.25 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/style-rtl.css 2.46 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 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.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.95 kB 0 B
build/list-reusable-blocks/index.js 3.12 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.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 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.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@jameskoster
Copy link
Contributor

As discussed in #19259, in the interest of keeping the toolbar lean, is it worth considering the idea of grouping template part actions here? I appreciate it's beyond the scope of this PR specifically, but aiming for something like this makes some sense to me and is perhaps worthy of discussion here:

tp

To bring things back in scope, maybe something like this could work as an interim:

scope

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Noting that the code looks good to me. I think it makes sense to move the button beside the input field as suggested, especially if we are going to iterate to use a different dropdown menu there in the future

@noahtallen
Copy link
Member

I also think it would be nice to have an option to "create new" (or even "duplicate") from this dropdown, but that doesn't have to block this :)

@Addison-Stavlo
Copy link
Contributor Author

@jameskoster

is it worth considering the idea of grouping template part actions here? I appreciate it's beyond the scope of this PR specifically, but aiming for something like this makes some sense to me and is perhaps worthy of discussion here:

Yes, I think that will definitely make sense especially as we expand on this to add the "create new" and "duplicate" type options. Having these related options in the same dropdown will definitely help keep things organized and the toolbar lean.

To bring things back in scope, maybe something like this could work as an interim:

While it may be worth putting the current picker under this dropdown style, I don't think it makes sense to use a different component/design for sorting through, previewing, and selecting template parts between the placeholder and the instantiated block. Theoretically, picking template parts in either of these places should have the same requirements and using the same component also allows us to keep things more consistent, maintainable, and updatable in the future as well. The design of the picker itself can definitely be iterated upon in the future, but here I think it is best to make the current picker accessible from this flow as opposed to implementing a separate system.

I would also be wary of a design for selecting template parts that lists them without a means of filtering. As a users library of template parts grows, the ability to search/filter them by name/theme/type increases. Without this the list can become long and hard to manage, and if we only show 'related' template parts without the ability to change that filter then we are forcing a restriction on what template parts the user is allowed to switch to, which seems unreasonable.


@noahtallen

I think it makes sense to move the button beside the input field as suggested, especially if we are going to iterate to use a different dropdown menu there in the future
I also think it would be nice to have an option to "create new" (or even "duplicate") from this dropdown, but that doesn't have to block this :)

I think moving it into the dropdown might be a bit weird before we have other options like create/duplicate. Id say we either:

  • Merge this with the current button in the toolbar, and follow-up soon after with adding create/duplicate functionality and moving them all into a dropdown.
  • Or just keep this open for a bit while we add create/duplicate functionality and the grouped option dropdown system.

Either way seems fine to me.

@noahtallen
Copy link
Member

I don't think it makes sense to use a different component/design for sorting through, previewing, and selecting template parts between the placeholder and the instantiated block

I agree with this. I will say that the options like "duplicate" or "create" would be nice to have for existing blocks and might not be useful for new blocks, so that could be one difference in the ultimate design.

Merge this with the current button in the toolbar

I was thinking that we could use this design for where the button lives

scope

Perhaps changing the icon, and then the actual content of the dropdown is the same as what exists? That way we can reuse the existing component for now.

Then we're in a spot where we can change template parts without much effort, and the interaction is very similar to what it will ultimately be (without having to immediately implement the new designs which are being worked on)

@Addison-Stavlo
Copy link
Contributor Author

I will say that the options like "duplicate" or "create" would be nice to have for existing blocks and might not be useful for new blocks, so that could be one difference in the ultimate design.

Definitely, the "create" option for the placeholder is separate from the picker there as well. I would imagine that dropdown as noted could have three buttons "Choose another", "Duplicate", and "Create new". Where the "Choose another" button would open the existing picker that would be shared between the two flows for consistency.

Perhaps changing the icon, and then the actual content of the dropdown is the same as what exists?

Yeah I think that makes sense here. Get rid of the separate new button in the toolbar, move it into the name panel with the dropdown icon, and have it open similarly from there. Then in the future we can update that dropdown to have three separate options as we add those functionalities instead of just opening the picker.

@Addison-Stavlo
Copy link
Contributor Author

e86552a updates to using the dropdown icon and compressing that into the same section as the name panel:

Screen Shot 2020-09-04 at 1 19 43 PM

Screen Shot 2020-09-04 at 1 19 53 PM

@Addison-Stavlo Addison-Stavlo force-pushed the try/selecting-different-template-part branch from e86552a to cae1768 Compare September 4, 2020 17:25
@Addison-Stavlo
Copy link
Contributor Author

and rebased.

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

The template part switching experience looks good and it sets a solid foundation for future work, given what @jameskoster and @noahtallen have mentioned earlier 🚀

  • Chrome
  • Firefox
  • Edge
  • Safari

Notes:

  • Do we have a stance on unit tests for site editing blocks?
  • IE11 is looking wonky, but from my understanding, that's not something we're actively supporting for site editing?

Screen Shot 2020-09-08 at 11 17 19 AM

@jeyip
Copy link
Contributor

jeyip commented Sep 8, 2020

+1 from me if we're not supporting ie11

@Addison-Stavlo
Copy link
Contributor Author

IE11 is looking wonky, but from my understanding, that's not something we're actively supporting for site editing?

Im not 100% sure on that, it would be nice to fix it for that as well though if able. Im assuming this funkyness isn't present in current master?

Do we have a stance on unit tests for site editing blocks?

I haven't seen much regarding this. Currently for Template Parts we have some e2e tests in https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/specs/experiments/template-part.test.js

@jeyip
Copy link
Contributor

jeyip commented Sep 8, 2020

Im assuming this funkyness isn't present in current master?

Oh that's a great callout. 🤦 I assumed it had to do with the new dropdown icon because of the look of the bug, but I didn't cross-check check master. I'll do it now.

@jeyip
Copy link
Contributor

jeyip commented Sep 8, 2020

This is what's currently on master. It looks like it's the template part name input interrupts the block toolbar layout, and not the icon in this PR. I think it's fair to tackle the bug in a follow-up 👍

Screen Shot 2020-09-08 at 12 04 33 PM

@jeyip
Copy link
Contributor

jeyip commented Sep 8, 2020

I haven't seen much regarding this. Currently for Template Parts we have some e2e tests in https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/specs/experiments/template-part.test.js

Thanks for the heads up. I'll think I'll raise the question about unit testing more broadly in #cylon

@noahtallen
Copy link
Member

IE11 is looking wonky, but from my understanding, that's not something we're actively supporting for site editing?

I don't think it's worth worrying about right now

@Addison-Stavlo Addison-Stavlo force-pushed the try/selecting-different-template-part branch from a5bd923 to e2d7e04 Compare September 9, 2020 01:40
@Addison-Stavlo Addison-Stavlo merged commit bd8bfea into master Sep 9, 2020
@Addison-Stavlo Addison-Stavlo deleted the try/selecting-different-template-part branch September 9, 2020 02:15
@github-actions github-actions bot added this to the Gutenberg 9.0 milestone Sep 9, 2020
@ZebulanStanphill ZebulanStanphill added the Needs Accessibility Feedback Need input from accessibility label Sep 9, 2020
@ZebulanStanphill
Copy link
Member

Pinging @afercia to check this out. (Sorry for so many pings lately.) I think this needs some accessibility feedback. A chevron doesn't seem like the best way to indicate a button to switch the template part. Why not a pencil "Edit" icon? Also, I'm not sure if the chosen control labels are good enough. "Choose another" should probably be "Choose another template part", shouldn't it?

@afercia
Copy link
Contributor

afercia commented Sep 9, 2020

Can anyone be so kind to please update, or better add, test instructions?

  • is there the need to install a specific theme to test this UI?
  • if so, where this theme is available?
  • what are the specific steps to follow to make this UI appear?

I'd like to remind everyone that this is an open source project, where any contributor is supposed to have all the necessary information be able to test a pull request, get familiar with it, and bring in their contributions if they like. Thank you 🙂

@Addison-Stavlo
Copy link
Contributor Author

Pinging @afercia to check this out. (Sorry for so many pings lately.) I think this needs some accessibility feedback. A chevron doesn't seem like the best way to indicate a button to switch the template part. Why not a pencil "Edit" icon? Also, I'm not sure if the chosen control labels are good enough. "Choose another" should probably be "Choose another template part", shouldn't it?

Agreed. At this point we are adding the functionality with the assumption that the design and accessibility will be reiterated. This is definitely not an end-state for this interaction, and it will need to be reorganized as support for other interactions are added. Given that this is in a more temporary state and that this is still an experiment under constant iteration, we have not stalled to polish a perfect design and interaction at this point in time but aim to reiterate in that direction as we continue to move forward.


Can anyone be so kind to please update, or better add, test instructions?

Id be glad to help! My apologies for the test instructions being very short, the scope of this change is very small within the experiment, but if you are coming into the Site Editor experiment as a whole there are quite a bit of missing pre-existing context.

  1. You will need to enable the "Full Site Editing" experiment under Gutenberg -> Experiments in wp-admin. (We recommend NOT enabling "Full Site Editing Demo Templates" as having done this may interfere with other Site Editor testing in other areas, and may require a cleaning the environment if it had been enabled)

is there the need to install a specific theme to test this UI?

  1. Install and activate a block based theme that includes template parts. There are a few available https://github.com/WordPress/theme-experiments such as twentynineteen-blocks.

what are the specific steps to follow to make this UI appear?

  1. Load the "Site Editor" (there should be a button in wp-admin just under Gutenberg once the experiment is enabled). The top section of the template in the editor will be a header template part. Selecting this Template Part block will allow the toolbar to appear as shown.

or

  1. Loading in either the Post Editor, you can insert a block called "Template Part" that becomes available with the experiment enabled. This block is added as a placeholder with the options to select an existing template part or to create a new blank one. Once one of these steps is done, you can access the toolbar from there as well.

Please let me know if you have any other questions or need further assistance.

@noahtallen
Copy link
Member

Why not a pencil "Edit" icon?

You would editing be editing the template part at the time, so switching it is a different interaction from editing. But I totally agree that the chevron might not be ideal.

And as always, we are happy to let others choose the icons and copy for these things! I will totally recognize that my strengths as a developer are not in choosing the text which best describes an interaction.

Can anyone be so kind to please update, or better add, test instructions?

I think an action item here is to create a documentation page which we can link to on FSE-related PRs. That way, it's easy for anyone to get started with it! :)

@afercia
Copy link
Contributor

afercia commented Sep 10, 2020

@Addison-Stavlo thank you. That helped.

My main difficulty in testing this UI was finding a way to actually select the template part. That's really difficult to understand. At the end, I used the Breadcrumbs bad at the bottom.

My main concern with this UI is that inserting an input field within the toolbar breaks the ARIA toolbar pattern, which amongst other things implements arrows navigation and the "roving tabindex" pattern. This has been noted also in other issues and PRs as there are already other UIs designed to use unexpected elements within the toolbar. One of them, already in production, is the Image block "editing tools" UI, which breaks interaction with the toolbar. See for example: #24766, #24676, , #24021, #23375, #24805. This pattern needs to be avoided. The accessibility team already discussed this topic in the latest weekly meeting and it will probably discuss it again in the next one to explore alternative options.

@noahtallen
Copy link
Member

My main difficulty in testing this UI was finding a way to actually select the template part. That's really difficult to understand. At the end, I used the Breadcrumbs bad at the bottom.

Totally agree! We did some investigation into this issue in #22064, but are still iterating on the best way to do so.

@ZebulanStanphill
Copy link
Member

Come to think of it, why is this dropdown even a thing in the first place? Wouldn't it make more sense to insert template parts from the inserter, similar to reusable blocks? I know there's no room for another tab, but to me that just says we shouldn't have used tabs in the first place. It seems weird to me that we're switching template parts from the toolbar.

@Addison-Stavlo
Copy link
Contributor Author

Wouldn't it make more sense to insert template parts from the inserter

Yes, this has been discussed as well (I think I even had a draft PR to add a tab for template parts to the inserter that way, and was prompted to add previews to the placeholder block instead). Currently, we can use the inserter to insert the 'Template Part' placeholder block, which has a button to trigger the preview/selection interface.

why is this dropdown even a thing in the first place?

So template parts can be changed once the block has already been initiated from the placeholder. Instead of inserting an entirely different template part block and deleting the unwanted one, we have an option to switch which one the block references.

I know there's no room for another tab, but to me that just says we shouldn't have used tabs in the first place. It seems weird to me that we're switching template parts from the toolbar.

Yeah, I cant verify if the toolbar will be the final resting place or not. Discussion may move this interaction to the top bar, sidebar, or elsewhere. For now, we just need it 'somewhere' to aid in development and testing. Having it here for a day has already helped point out a silly bug in renaming that was otherwise unnoticed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Accessibility Feedback Need input from accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants