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

Navigation block: Add horizontal movers #16637

Closed
draganescu opened this issue Jul 17, 2019 · 10 comments · Fixed by #16615
Closed

Navigation block: Add horizontal movers #16637

draganescu opened this issue Jul 17, 2019 · 10 comments · Fixed by #16615
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@draganescu
Copy link
Contributor

Some blocks have inner blocks that need to be moved horizontally, for example the menu block, the gallery block and the columns block. The block mover assumes that only vertical rearrangement is possible, so we need to implement a way to allow horizontal rearrangement.

@draganescu draganescu added [Type] Enhancement A suggestion for improvement. [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P labels Jul 17, 2019
@draganescu draganescu self-assigned this Jul 17, 2019
@draganescu draganescu added the [Status] In Progress Tracking issues with work in progress label Jul 17, 2019
@mtias
Copy link
Member

mtias commented Jul 30, 2019

This would be the addition of an orientation={ 'horizontal' } or just a boolean horizontal property of InnerBlocks. The movers would be just one aspect of it.

cc @felixarntz as this seems relevant to what you have been doing in AMP-stories territory.

@mtias mtias added the [Block] Navigation Affects the Navigation Block label Jul 30, 2019
@mtias mtias mentioned this issue Aug 21, 2019
5 tasks
@noisysocks noisysocks changed the title Allow horizontal movers on inner blocks Navigation block: Add horizontal movers Sep 24, 2019
@draganescu
Copy link
Contributor Author

draganescu commented Sep 24, 2019

At this moment the BlockMover component is used in the BlockListBlock component and it is rendered in a shared container with the BlockEdit component. This is great for positioning the movers absolutely from the BlockEdit itself. It is not so great for having the BlockMover render inline next to BlockEdit.

Here are some solutions:

  • duplicate the BlockMover code in BlockEdit and render it selectively based on some orientation flag, before BlockEdit for orientation:vertical and after BlockEdit for orientation:horizontal. This is OK, but the code in BlockMover becomes a bit messy.
  • use display:grid on the shared container and place BlockMover on a column based on some orientation flag. This works, more or less, but it requires a redo of some of the current BlockMover CSS positioning code and raises accessibility issues as well.
  • drop BlockMover into BlockEdit and have it render by default as vertical then leave each block do it's thing for mover variations. This way each block can decide to not render the default BlockMover and instead render something else. In the case of NavigationMenu render the mover inline with each NavigationMenuItem.

There are some other options though.

First, it bothers me that while vertical movers are standard horizontal movers vary: the gallery has horizontal arrows positioned absolutely on top of gallery items, the navigation block has horizontal movers positioned inline next to menu items. It feels off somehow. So, one option would be to not have horizontal BlockMover inline but absolutely positioned, like this:

horizontal_mover_2

or even simpler like this:

Screenshot 2019-09-24 at 13 33 01

Second, maybe the whole movers thing could be improved for all the blocks if we implement something else in NavigationMenu such as:

  • toolbar movers like @mtias suggested

image

So, are we sold with this idea that the NavigationMenuItem 's horizontal movers should be inline and look like the ones implemented by the Gallery block (which doesn't implement InnerBlocks but some custom way)?

Buttons with arrows over a navigation menu item

Because if we are then, imho, the best way is to implement them custom in NavigationMenuItem just like Gallery does for its items and not as a general InnerBlocks feature.

@talldan
Copy link
Contributor

talldan commented Sep 26, 2019

  • duplicate the BlockMover code in BlockEdit and render it selectively based on some orientation flag, before BlockEdit for orientation:vertical and after BlockEdit for orientation:horizontal. This is OK, but the code in BlockMover becomes a bit messy.

In order to get this option working for both the Nav Menu block and a Gallery that has inline blocks (and maintain tab order), I think there would need to be two additional BlockMovers rendered, one before the BlockEdit and one after. Configuration could be something like:

<InnerBlocks
  useInlineMovers="before|after"
  isHorizontalLayout
/>

The styling for these movers will be pretty complicated, which is why I think a BlockMover component that could be dropped into the block's edit will be easier to work with. I know the drawback with that is that it doesn't offer the out-of-the-box functionality that we want to provide block developers with.

@jasmussen
Copy link
Contributor

In reading through this thread, it reads to me as if the biggest argument for duplicating the block mover code is that it makes it possible to apply some stylistic changes that are not possible currently, is that correct?

I love a challenge, so I would love to understand what style we want and what makes that possible with the current approach. I read this:

At this moment the BlockMover component is used in the BlockListBlock component and it is rendered in a shared container with the BlockEdit component. This is great for positioning the movers absolutely from the BlockEdit itself. It is not so great for having the BlockMover render inline next to BlockEdit.

What about the horizontal layout makes it hard to accomplish the horizontal movers based on what we have today?

If it helps, I can create an example PR where I unhide the movers from the recently merged Social Links block, and style those to be horizontal.

Note I do think in general that the block mover needs a lot of love. But those benefit deserve to benefit block movers everywhere, not just in nested blocks, IMO.

@draganescu
Copy link
Contributor Author

After a lot of fiddling with the CSS ( cc @jasmussen ) in which I basically added a wrapper and used a CSS grid to move the movers around I gave up and found no way to make them stay in the same place in the DOM and look like this:

Therefore I went ahead and updated #16615 to have the BlockMover component be passed into any kind of BlockEdit. There each block which needs horizontal movers can render them wherever it sees fit:

The styling is crap because the MenuItem component is incomplete and has some outdated UI.

@talldan
Copy link
Contributor

talldan commented Sep 27, 2019

In reading through this thread, it reads to me as if the biggest argument for duplicating the block mover code is that it makes it possible to apply some stylistic changes that are not possible currently, is that correct?

@jasmussen I personally think tab order is the main issue. I'm sure there's lots that's achievable using purely CSS, but afaik CSS can't solve that the mover is after the content in the nav menu item, and before it everywhere else.

Happy to be proven wrong though 😄

jasmussen added a commit that referenced this issue Sep 27, 2019
This is more of a proof of concept and experiment, than it is final PR. So *do not merge as is*.

We might find that we like this, in which case it might need a little polish, and _then_ we can ship it. But for now this PR has been created to help further discussion in #16637.

This PR does the following:

- Starts with Social Links, because that is the first block to "absorb child block UI", and therefore a good test-bed for this.
- Additionally, Social Links has been merged with _no mover controls_. This PR resurfaces them.
- It restyles, positions, and rotates the mover control to be horizontal.
- It restyles and positions the ellipsis menu.

The overall pattern it mimics, is that of gallery items, which also have horizontal movers, even though those are not yet technically child blocks.

Your feedback is much apprecated.
@jasmussen
Copy link
Contributor

Hey @talldan and @draganescu, thanks for blazing on with this, your work is very helpful in understanding the challenges involved, I know I've had a hard time understanding the intricacies. So these examples have been very helpful, to the point that I took a stab myself in #17627.

Basically it sounds like the primary motivator for making the block mover into a subcomponent, was to show it inline and to the right of the child item. For that reason I took a different approach, kept them where they are and just re-did them, kind of like Andrei did in one of the GIFs here: #16637 (comment)

I don't necessarily think this is the IDEAL solution, but it is a CSS-only solution that we can refine as we go, and it keeps with the idea of keeping the block, notably the innerblocks feature, as simple as we can for developers.

Would very much appreciate your thoughts.

@draganescu
Copy link
Contributor Author

This issue is almost completed by #16615 which is missing a final review.

@ellatrix
Copy link
Member

@draganescu Looks like the movers are not showing correctly in the top toolbar. See #19565.

@draganescu
Copy link
Contributor Author

Hi @ellatrix I am afraid I'll need to get more up to date with moving the movers to the toolbar and what that means for how they work. I'll check the issue you opened and comment there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
5 participants