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

Try/plugin header toolbar #17198

Closed

Conversation

ryanwelcher
Copy link
Contributor

@ryanwelcher ryanwelcher commented Aug 26, 2019

Description

This PR implements SlotFill to render the <BlockControls /> and additionally exposes a new PluginHeaderToolbar SlotFill. This PR addresses #16988.

How has this been tested?

npm run test ran locally.

Screenshots

Types of changes

This PR introduces a change in how the BlockControls are added and should have no other impact. As we are adding the BlockControls immediately after exposing the Slot, any new items added would appear after them.

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.

@ryanwelcher ryanwelcher added the [Feature] Extensibility The ability to extend blocks or the editing experience label Aug 26, 2019
@ryanwelcher ryanwelcher requested a review from talldan as a code owner August 26, 2019 15:40
@ryanwelcher ryanwelcher self-assigned this Aug 26, 2019
@ryanwelcher ryanwelcher requested a review from gziolo August 26, 2019 15:42
@talldan
Copy link
Contributor

talldan commented Aug 28, 2019

Hi @ryanwelcher. Thanks for taking this on.

What would be really useful for this PR and the associated issue is an explanation of what it would be used for. Screenshots or designs would also be great.

I can think of a couple of issues.

  • How does this work with the Top Toolbar option? Will it allow all the already existing UI in this location to still behave responsively if plugin developers can add extra UI components here.
  • How might it interfere with other proposals in the project, like the one to add the post title to the toolbar - Add Block: Title #11553

This would also require some documentation.

@talldan talldan added the [Status] Needs More Info Follow-up required in order to be actionable. label Aug 28, 2019
@gziolo
Copy link
Member

gziolo commented Aug 28, 2019

Some things to consider:

  1. When using Gutenberg on smaller screens the setting for the top toolbar is ignored and the block toolbar is rendered on the bottom of the block:
    Screen Shot 2019-08-28 at 09 00 52

  2. When using Gutenberg on medium screens the setting for the top toolbar is ignored and the block toolbar is rendered at the top of the block:
    Screen Shot 2019-08-28 at 09 07 55

  3. When using Gutenberg on large screens the setting for the top toolbar works as expected but sometimes the block toolbar is rendered in its own row:
    Screen Shot 2019-08-28 at 09 05 12

I guess this works this way to fit all items in the header. @jasmussen and @mtias should know more. While it seems like a great idea to give the freedom to add your own items to the header, it might be difficult to ensure it is visible on smaller screens. Another challenge here is that we wouldn't allow every possible item to be added to the toolbar to ensure everything is accessible like in this example:
https://www.w3.org/TR/wai-aria-practices/examples/toolbar/toolbar.html
Screen Shot 2019-08-28 at 09 15 20

So this would need to be something which handles keyboard interactions. The example implementation in Reakit:
https://reakit.io/docs/toolbar/

@ryanwelcher
Copy link
Contributor Author

ryanwelcher commented Aug 28, 2019

@talldan @gziolo thank you for your insights on this!

I think we have two areas to consider here:

  1. Whether it makes sense to use SlotFill to render the BlockControls instead of the logic that was there.
  2. Do we expose that SlotFill for external use?

Personally, I like the idea of using SlotFill here as it provides a pretty simple solution to extending the HeaderToolbar from anywhere in the editor.

The larger issue is whether this needs to be exposed for use by plugin developers. In theory, I would say yes but it looks there are many combinations that need to be taken into account - especially around screen size and accessibility. If we're going to expose a SlotFill here, we'll need to either provide all of the props a component may need to render ( hasFixedToolbar, isLargeViewport etc ) or alternatively, maybe there is a solution using SlotFill that are conditionally rendered by viewport size?

Perhaps, the PR can be changed to use the SlotFill but not expose it until we have a better solution?

As GB matures and expands to other sections of the WP admin, there are going to be more and more requests for extension points. It would be great to be able to have some ideas for areas like this that we can apply to other areas while building them out.

@matthewbrent
Copy link

As I opened the issue and i'm an unbiased party here, i'd like to weigh in my two cents.

Really appreciate you creating a PR on this @ryanwelcher. To understand the hesitations on this, i'm wondering where the line is between what WordPress provides out-of-the-box and where things are opened up for plugin and theme developers.

In previous versions of WordPress, it definitely felt like there was an openness around the system and that content developers were encourage to implement their own solutions and it was the developers responsibility to ensure ARIA accessibility and mobile responsiveness etc. In Gutenberg's initial launch, it was pegged as extensible yet in terms of extensibility, the GB editor is actually pretty closed currently.

I'm not sure if this is intentional or not, and i'm not complaining. Im trying to understand the strategy here and if there's anyway I can have some meaningful conversations to perhaps provide some more insight from a non-contributing developer.

As @ryanwelcher states, if the desire is to refactor other parts of the admin to React, then there's going to have to be far more extensibility options otherwise there's going to be a large majority of developers feeling put out.

@paaljoachim
Copy link
Contributor

Hey @ryanwelcher and others. It would be great with a status update on this PR.
Thanks!

@jeffpaul
Copy link
Member

@paaljoachim I think the status is that folks are unclear what the Gutenberg team wants on this specific topic. Can you help summarize what's needed as I'd love to see a solution to the problem of the linked issue land in Gutenberg in time for WP 5.5?

@jasmussen
Copy link
Contributor

Personally, I feel that it is important that when we open up new slot fills for developers to populate, we think very carefully about the consequences. Gzregorz outlines some of the challenges with making the top toolbar extensible — it's already a crowded area, and we forcefully hide specific known items on mobile for it all to fit. The full site editing experience ponders putting the page title and other options at the top center (#20877), and then there's the top toolbar option.

So I imagine the standstill on this one is not for a lack of appreciation of the work done, but rather that there isn't a clear path forward for the UI side, that accounts for all that needs to be accounted for when adding a new button to the toolbar.

I do appreciate the need for shops and plugins to tailor and customize the editor for a particular flow, so the addition of slot fills to specific places is fundamentally good. For example, I'm excited for #20929, so that the top left button can be customized in both appearance and behavior. There's the existing sidebar API option, and both the pre-publish experience and the post-publish dialog should be extensible as well.

@gziolo
Copy link
Member

gziolo commented Jun 12, 2020

I do appreciate the need for shops and plugins to tailor and customize the editor for a particular flow, so the addition of slot fills to specific places is fundamentally good. For example, I'm excited for #20929, so that the top left button can be customized in both appearance and behavior. There's the existing sidebar API option, and both the pre-publish experience and the post-publish dialog should be extensible as well.

I'm less excited about #20929 than you because it basically allows you to replace the W icon button with an indefinity number of fills that can contain anything you want. In contrast to other fills, the one you mentioned doesn't guide you towards the implementation that ensures a consistent experience for those who use the block editor. Just to give an example, while in the toolbar that is the point of this whole discussions, we have only toolbar items which ensure that they look similar and have the same behavior or semantics for accessibility purposes - arrow key navigation between them, one tab to move focus outside the toolbar, one shift+tab to move focus back to the last active item.

The interesting conclusion is that you can prepend any number of fills in the Gutenberg plugin just before the block toolbar as of today, but technically they won't be part of the toolbar experience.

@ryanwelcher ryanwelcher force-pushed the try/plugin-header-toolbar branch from a877b4d to 3362a00 Compare June 12, 2020 18:43
@ryanwelcher
Copy link
Contributor Author

ryanwelcher commented Jun 16, 2020

I have been thinking about this implementation and wanted to share some thoughts and a different approach to this.

I believe we should be more purposeful about what is placed into Slot locations. Generally speaking, opening a spot to place anything is going to lead to very bad user experience and bad times overall.

This new location should be restricted to the same type of controls that exist now. I propose that the Fill return a ToolbarItem that set to use a custom DropDown using a Button with isPrimary set to false.

In addition to the className, classContentName and position related props, the fill will accept a prop for the icon to display and a renderContent prop that will be passed to the inner DropDown component. This will keep each item added here standardized and prevent abuse for this Slot.

Example implementation:

const render = () => (
    <PluginHeaderToolbar
        className="my-plugin"
        classContentName="my-plugin-content"
        position="bottom left"
        renderContent={() => <div>Content to render</div>}
      />
);
registerPlugin("my-plugin", { render, icon: "twitter" });

Screenshot:
plugin-header-toolbar

Screenshot with Block Controls in the Toolbar:
plugin-header-toolbar-block-toolbar

In addition to the changes to SlotFill, I also propose that this slot is only rendered when isLargeViewport is true. These items should be considered secondary and treated similarly to the ToolSelector as they are not crucial to content editing.

I have updated this PR to reflect the above.

@ryanwelcher
Copy link
Contributor Author

@gziolo I'd love any feedback you may have on this PR.

As a side note, with this new approach, the slot might be better named something along the lines of PluginHeaderToolbarButton

@jeffpaul
Copy link
Member

Note that I left a comment in the linked issue to try and move the discussion on approach out of the PR and into the issue so that we can agree on approach there and worry about fine-tuning a solution in the PR.

@gziolo
Copy link
Member

gziolo commented Jun 17, 2020

@jeffpaul – let's discuss in the parent issue as you suggested, I will copy comment from @ryanwelcher and respond there.

Base automatically changed from master to trunk March 1, 2021 15:42
@lukecarbis
Copy link
Contributor

Just to address the issue of "it's already a crowded area, and we forcefully hide specific known items on mobile for it all to fit":

Many plugins are already using this area without a slot fill via various hacky methods. See Extendify and Google AMP as examples (more info: #16988 (comment)).

Better that we provide a slot fill which gives the user a settings for which controls to show in this area.

@ndiego
Copy link
Member

ndiego commented Jun 1, 2023

Now that the Editor is far more stabilized than it was back in 2019 when this PR was first created, I think it would be a good time to revisit this PR. As the last commenter notes, plugin authors have already been using very hacky methods to add buttons to this section of the UI. Having a formal way of doing this, with proper guardrails, would be a huge benefit to the project while providing the extensibility that the extender community is clearly looking for.

@priethor
Copy link
Contributor

@ryanwelcher, do you think this PR is actionable with so many changes in the Editor in these years?

@ryanwelcher
Copy link
Contributor Author

I'll update the PR and take a look @priethor. I still there is a need here.

@ryanwelcher
Copy link
Contributor Author

@priethor @gziolo @ndiego @gziolo I have moved this PR to #52261 and updated it as needed. I'm happy to close this but I'd like to be sure we have the conversation available if possible

@ryanwelcher
Copy link
Contributor Author

Closing in favor of #52261

@ryanwelcher ryanwelcher closed this Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Status] Needs More Info Follow-up required in order to be actionable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants