plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 574 forks source link

Grid block in core + primitive Container block #3180

Closed sneridagh closed 10 months ago

sneridagh commented 2 years ago

Backport of @kitconcept/volto-blocks-grid (7.x.x) features, using Volto's default blocks-in-block architecture. This change will render old grid blocks incompatible with this one, unless a migration is made. However, coexistence between old grid and new ones is possible.

UPDATE 01.Oct.2022: Teasers moved to a separate PR: #3706 so they can go on its own lifecycle. I left the old implementation here for testing purposes. They should be removed after #3706 is merged.

UPDATE 09.June.2023: After discussion in the Beethoven Sprint these decisions were made for short term (affecting this PR):

Future:

An improved drag-and-drop system will be needed and the ability to "move" blocks from outside (main block engine column) to inside the Row/Column blocks, and from one row/column block to another.

Minimum Viable (mergeable) Product

Improvements

Nice to have

netlify[bot] commented 2 years ago

Deploy Preview for volto ready!

Name Link
Latest commit fdd10fbc96bd048cd72bbc9274b3382d8ad768b2
Latest deploy log https://app.netlify.com/sites/volto/deploys/649be25a0217660008e322c4
Deploy Preview https://deploy-preview-3180--volto.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

nileshgulia1 commented 2 years ago

An improved drag-and-drop system will be needed and the ability to "move" blocks from outside (main block engine column) to inside the Row/Column blocks.

Looks promising🔥

cypress[bot] commented 1 year ago

Passing run #5957 ↗︎

0 511 20 0 Flakiness 0

Details:

Improve test
Project: Volto Commit: 5701066f30
Status: Passed Duration: 12:50 💡
Started: Jun 27, 2023 4:23 PM Ended: Jun 27, 2023 4:36 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

djay commented 1 year ago

@sneridagh can you replace the x icon with a trash icon as per https://github.com/plone/volto/issues/3816 ?

stevepiercy commented 1 year ago

@sneridagh also consider removing the X, as it looks to be redundant to an existing trash can icon to the right, per https://github.com/plone/volto/issues/3816#issuecomment-1298047935

sneridagh commented 10 months ago

@stevepiercy please could you take a look at the docs?

ksuess commented 10 months ago

I think both is of interest: image as it is in a grid and images cropped to predefined aspect ratio in grid. How about offering both full height and aspect ratio to the user (via image alignment)?

image_options


cropped to aspect ratio grid_image_aspect_ratio

image as it is, uncropped grid_image_full_height

sneridagh commented 10 months ago

@ksuess @davisagli The policy in Volto core has always been "don't make the user think" for defaults, being the default the one he most likely would want, and looks better by default (even if it's opinionated). If the integrator wants to allow other approaches (like the one you proposed), then either provide a handler (widget) for it, or a variant as preset (even more straightforward, and don't make the user think what widget combination will provide the result he's looking for).

Defaulting to a fixed aspect ratio provides consistency (if you force it everywhere) to your site images and a streamlined and better look overall.

ksuess commented 10 months ago

@ksuess @davisagli The policy in Volto core has always been "don't make the user think" for defaults, being the default the one he most likely would want, and looks better by default (even if it's opinionated). If the integrator wants to allow other approaches (like the one you proposed), then either provide a handler (widget) for it, or a variant as preset (even more straightforward, and don't make the user think what widget combination will provide the result he's looking for).

Defaulting to a fixed aspect ratio provides consistency (if you force it everywhere) to your site images and a streamlined and better look overall.

OK, I was just irritated by the fact, that in edit mode the image of an image block appears with a fixed aspect ratio of 16:9 and in view mode it does not but unchanged at all. I assume you want the image of an imge block to come per default with an aspect ratio 16:9?

edit mode: grafik

view mode: grafik

stevepiercy commented 10 months ago

OK, I was just irritated by the fact, that in edit mode the image of an image block appears with a fixed aspect ratio of 16:9 and in view mode it does not but unchanged at all. I assume you want the image of an imge block to come per default with an aspect ratio 16:9?

I would expect both the view and edit modes to be WYSIWYG with the same aspect ratio applied.

ksuess commented 10 months ago

OK, I was just irritated by the fact, that in edit mode the image of an image block appears with a fixed aspect ratio of 16:9 and in view mode it does not but unchanged at all. I assume you want the image of an imge block to come per default with an aspect ratio 16:9?

I think it's just a CSS rule missing. By now it seems to assume a .grid-image-wrapper arround images of image blocks? I don't see this. Where does the wrapper come from? In plain vanilla Volto it looks like this: grafik

https://github.com/plone/volto/blob/2a465f8c7d94b47c4e39fe14cf6cb0ecdce164e3/theme/themes/pastanaga/extras/grid.less#L369-L376

sneridagh commented 10 months ago

@ksuess let me see, they should match, for sure!

sneridagh commented 10 months ago

@stevepiercy I added a couple of paragraphs more, I realised that the props apply to the container too, and added two more, in case you want to review.

sneridagh commented 10 months ago

After this one, it's ready for review and merge!!

sneridagh commented 10 months ago

@davisagli I realised that if the slate add-on was responsible for adding its own config, why don't make it responsible to add the config for the grid too?

So here we go, last commit. Let me know what do you think. I removed the finalize thinggie too.

pnicolli commented 10 months ago

@ksuess @davisagli The policy in Volto core has always been "don't make the user think" for defaults, being the default the one he most likely would want, and looks better by default (even if it's opinionated). If the integrator wants to allow other approaches (like the one you proposed), then either provide a handler (widget) for it, or a variant as preset (even more straightforward, and don't make the user think what widget combination will provide the result he's looking for).

Defaulting to a fixed aspect ratio provides consistency (if you force it everywhere) to your site images and a streamlined and better look overall.

@sneridagh Couple days late to the party here probably, but I don't fully agree here. I agree on having sane and good looking defaults, therefore I am totally ok with having an aspect ratio as the default and having it configurable with a css property. What I would want though is for the user to be able to choose to unset the aspect ratio. Just unset, not configure. I have seen use cases for both set and unset ratio in the same site, I think we should allow that. I just read the comments here and could not try the PR so I am answering based on what I understood. If the option is actually there, then I'm fine :)

stevepiercy commented 10 months ago

For convenience, new docs preview:

avoinea commented 10 months ago

Add a grid with an image and a teaser block, the make the image full width:

Screenshot from 2023-06-20 13-39-39

avoinea commented 10 months ago

Add a grid block with an image and text block inside, the copy some text with multiple paragraphs from https://www.lipsum.com/feed/html and paste it within the text block, no more 4 block limit:

Screenshot from 2023-06-20 13-44-40

After removing the text blocks:

Screenshot from 2023-06-20 13-46-17

avoinea commented 10 months ago

The same behavior if you un-bullet bulleted text:

Screenshot from 2023-06-20 13-55-11

Screenshot from 2023-06-20 13-55-21

davisagli commented 10 months ago

@pnicolli Yes, it's simple to unset the aspect ratio in CSS, and that is mentioned in the docs now.

pnicolli commented 10 months ago

@davisagli Yeah but I meant unsetting via Sidebar as a user, not via config.

sneridagh commented 10 months ago

@avoinea it should be fixed now. Give it a try! Anyways, I will give it another look tomorrow. I still think that some of the fixes can be improved.

djay commented 10 months ago

@sneridagh I think the UX might need a little more work. If you want to create a set of columns you might use a vertical container inside a grid block.

This screenshot is a grid block inside a grid block but the problem is going to be the same right? the control bars are going to overlap. And I think it's going to get confusing which control bar relates to which container. Also if you are dragging, are you dragging the inner most block or one of the containers? and then all of this has to work on mobile too.

Screen Shot 2023-06-21 at 5 13 50 pm

I can give it some thought on what a more sustainable UX solution might be for add, delete and settings for multi-containment scenario might be, or else will you ask @albertcasado?

sneridagh commented 10 months ago

@djay we agreed on these terms during the sprint:

UPDATE 09.June.2023: After discussion in the Beethoven Sprint these decisions were made for short term (affecting this PR):

@sneridagh I think the UX might need a little more work. If you want to create a set of columns you might use a vertical container inside a grid block.

We discussed a lot of scenarios where we can go with the current building blocks, as a community we should explore them from now on. Personally I'm not much interested in the containers in containers use case, as I see it as a pitfall where you open and endless set of issues of all kind, but I will give a try to more powerful ideas that came up from the discussions. I personally expect that this leads to a new breed of container blocks that enhances the current Volto ecosystem.

This screenshot is a grid block inside a grid block but the problem is going to be the same right? the control bars are going to overlap. And I think it's going to get confusing which control bar relates to which container. Also if you are dragging, are you dragging the inner most block or one of the containers? and then all of this has to work on mobile too.

Screen Shot 2023-06-21 at 5 13 50 pm

I can give it some thought on what a more sustainable UX solution might be for add, delete and settings for multi-containment scenario might be, or else will you ask @albertcasado?

As said, the containers in containers is just another use case that should be explored. The current PR only fixes the use case for the Grid block as it was in the add-on. You are correct that then we will have to put in place a nicest "toolbar" for the container in place, only making available the current one you have selected. However this is a major problem that we should improve in the future, hopefully with the Quanta block toolbar in place (no this poor man replacement) and for this we will need to evolve and improve the current solution. I don't see now how we can do that, except to provide a pluggable toolbar, when you can replace it completely with your own control toolbar (or extend the current one).

I can give a quick try on this last one as an improvement to the current primitive, although could also be an improvement to the primitive to be made in the future, no breaking change involved...

We briefly went through the possible UX for it, all pointing out to the Quanta blocks toolbar, and that should be enabled when you select the inner blocks (for the inner block) very much the same as WP or Elementor does nowadays... For the rest, we didn't had time to explore much more the use case.

djay commented 10 months ago

Personally I'm not much interested in the containers in containers use case, as I see it as a pitfall where you open and endless set of issues of all kind

Sure and I totally get why some sites might prevent columns inside columns scenario. But, a columns block is already containers inside containers and a columns block is necessary. By columns block I mean horizontal container containing vertical containers. This is the basic layout block in WP and its really what should be in plone core also. So containers in containers in unavoidable no matter what even if you if you ban column blocks inside column blocks

I get the point you want to move forward and let people experiment. However what is the goal of the container primative.

  1. make it easy for developers to create new blocks that contain other blocks
  2. give the user a consistent UI. e.g. how form-block allows adding sub items is the same as eea.columns adding subitems and managing containment.

So letting block developers come up with their own UI to deal with container inside container scenarios is both making it harder for them and resulting in inconsistent UI for the end users.

You can see how eea had to invent a way of solving containment navigation and adding themselves with their column block Screen Shot 2023-06-21 at 5 15 21 pm Screen Shot 2023-06-21 at 5 15 32 pm

So really it's a question of if there is anything we can add to volto in the near term to get something that does work well enough for containment in containment.

We briefly went through the possible UX for it, all pointing out to the Quanta blocks toolbar, and that should be enabled when you select the inner blocks (for the inner block) very much the same as WP or Elementor does nowadays...

I don't think the quanta toolbar solves the problem. Lets be clear about the problem

  1. You need a way to navigate up and down the containment heirachy in a clear and obvious way. esp there are properties on parent containers you need to get at.
  2. It needs to be clear what an add button does and how to go about adding something. when you have containers then you have both adding after the current block as well as how do I add a block inside any of the parent containers. Having 2 add buttons on screen at once is not great.
  3. you need that to work on mobile also

In the current container UI the problem is the container add and settings buttons being on screen the whole time. It has to change.

I think containment is always complicated. you see that in figma and other drawing programs. there is no magic bullet. But I did look at the latest gutenburg and I can see why they have evolved to make the choices they did. maybe not perfect but I think it solves the problems above so I think it's ok to copy

Screenshot_2023-06-21-19-06-25-921_com android chrome

anyone can try on https://wordpress.org/gutenberg/

if we had the global add after button and the containment breadcrumbs and the mobile container drill down clicks. that would be a min to solve the problem I suspect. all the rest is bonus. Or maybe there is a way to do better than what they have?

djay commented 10 months ago

@sneridagh More specifically these four things are needed to be built into the container primative IIMO that would make the it much more useful.

Screen Shot 2023-06-23 at 1 09 44 pm
  1. have a very clear indication of selection.
    • Currently if a block is selected in a grid then both the block and the grid have an outline showing they are both selected which is confusing.
  2. Get rid of all buttons on the screen that aren't related to what is currently selected.
    • The grid add/settings toolbar and the add button at the bottom (which doesn't seem to know about containment).
  3. Have an add button that will "insert after" the selected block.
    • In this case I've shown it consistent with the add button @davisagli added just in case thats easier to impliment. but it could also be in a quanta floating toolbar above the block, or like gutenburg, in the main toolbar not floating over the content at all. Personally I think the gutenburg's way has a lot going for it. It's going to work better on mobile and it's less likely to overlap something inside the block. It's also always in the same place so never have to look for it.
  4. Have a way to get to containers in case it's hard to click on them.

    • In this case I've put container breadcrumbs in the settings sidebar. But I actually think what gutenburg did with a bottom bar is better. That would give more space if the containment is complex and means you don't need to have the settings open.

    How hard would that be and what can we do to help?

sneridagh commented 10 months ago

@sneridagh More specifically these four things are needed to be built into the container primative IIMO that would make the it much more useful.

This is the first draft of the container, and I'm assuming that we will continue iterate over it in the future. I totally agree that the toolbar should be configurable as well, this is one thing I can take care of in this PR.

Screen Shot 2023-06-23 at 1 09 44 pm
  1. have a very clear indication of selection.

    • Currently if a block is selected in a grid then both the block and the grid have an outline showing they are both selected which is confusing.

We are so perusing the nesting ability (using the same core components in a nested way). The outter block is selected as well, thus it has outline. You could remove it via CSS only for your container use case or even for the Grid? Yes, however, our users never complained. I think it's informative and useful and gives context. Could be that in other use cases, it's redundant and it gets in the middle? could be.

  1. Get rid of all buttons on the screen that aren't related to what is currently selected.

    • The grid add/settings toolbar and the add button at the bottom (which doesn't seem to know about containment).

It doesn't. TBH, I haven't used the "new add button" feature while developing. I will take a look and remove it if a containment is present.

  1. Have an add button that will "insert after" the selected block.

    • In this case I've shown it consistent with the add button @davisagli added just in case thats easier to impliment. but it could also be in a quanta floating toolbar above the block, or like gutenburg, in the main toolbar not floating over the content at all. Personally I think the gutenburg's way has a lot going for it. It's going to work better on mobile and it's less likely to overlap something inside the block. It's also always in the same place so never have to look for it.

If you think your block is better with this, then implement it with a custom EditBlockWrapper.jsx the container allows to configure it for a reason. If I make the toolbar configurable you could also push for it easily.

The grid block works with the current toolbar, and it did all this time. We decided in the sprint not to change the grid block (in this PR) with new UX, so it will be the same as in @kitconcept/volto-blocks-grid.

  1. Have a way to get to containers in case it's hard to click on them.

    • In this case I've put container breadcrumbs in the settings sidebar. But I actually think what gutenburg did with a bottom bar is better. That would give more space if the containment is complex and means you don't need to have the settings open.

The breadcrumb/icon in the inner block title looks ok, but could be hard to implement... The sidebar knows nothing about its parent or if it's contained. That's why the toolbar has a "cog" icon handler in order to select it. Guttenberg has such a handler as well always visible (in the current selected inner block).

How hard would that be and what can we do to help?

So, as I see it now, you want to push for a better user experience for contained blocks and I share this, and I guess all the community. However, it's incredibly hard to get right, as example, the Guttenberg and Elementor use cases, where they changed their minds a couple of times.

We need some stability and because of that, we agreed that the core grid was 1:1 to the add-on. That's settled.

As scape hatch, we thought into this primitive container, so the community can explore and build their own contained blocks out of it in an unified way, also with the aim to help people building such a type of block. It's the first draft, and I expect that we evolve it and improve it over time. I myself committed to work in a better version of the container paradigm while I quite like the Elementor approach (without the bloated UX/widgets etc...). So I plan to continue working on improving it.

As I said, the containers in containers use case I'm not that interested because it just opens a Pandora box in all possible subjects. If you are interested in exploring it, let's do it.

I personally see the grid block as the minimum feature as in a horizontal container for Volto, having the very basics. But I do expect a new breed of add-ons with container blocks as a result.

So I would ask you to be able to move forward and be able to merge this PR as soon as possible. Then continue the containers discussion (and feedback from developing such containers) in another place? Maybe the forum? I'm very looking forward for it, really.

djay commented 10 months ago

@sneridagh

Consider the following layout. Multiple vertical containers that can hold any of the basic block types. Ie a Grid > Column > Block.

unselected

My clients need to create things like this. EEA do too, that’s why they created eea volto-columns-block. It’s a fairly basic requirement I would guess.

One thing you haven’t answered is how the container primitive would work with this common columns scenario. How the UI would work without block developers have to remove the primitive UI and replace it with their own inconsistent versions?

I can only see three options Prevent any 2 level containment. Ie grid block is allowed but columns is not Give up on WYSIWYG and allow extra spacing/chrome around containers all at the same time. This prevents two different containers toolbars from being on top of each other. Go to a proper selection model where you only show overlays about the currently selected block and not it’s parents.

I remember the original discussions about containment in tokyo. I know albert thinks containment is an antipattern. You seem to be trying to stick to that idea of not dealing with containers yet wanting to create a universal container primitive at the same time. You can’t have it both ways. If you have a container then people will use it to make containers inside containers. This is unavoidable. Not dealing with this issue now will just create more problems later. You would be releasing a half baked solution and causing churn.

The current version on the container primitive means volto-columns-block would have to disable your container toolbar due to the overlapping toolbar issues I mentioned. Out of the box the container primitive would give them this.

container_overlap

Then they need to add in another way to insert and add in another way to navigate up the the parent container, like they had to do once already

Screen Shot 2023-06-21 at 5 15 21 pm

And lhen later, once you get around to trying to solve this, which you will have to do eventually, they will then have to upgrade their block again with container primitive 2? We are going to have to do the same. And others too. You are creating a lot of work for other developers and an inconsistent user experience for end users. So I think for this reason trying to restrict containers to one level is also a bad idea.

I’d guess giving up on WYSIWYG is also out of the question. I believe it’s a core volto principle. Putting spacing/chrome/buttons around containers that make the items not appear how they would in the final version I believe is not the way to go.

So that only leaves showing chrome and overlays only on the current selected block (like every other WYSIWYG container based component editor I’ve found so far). Gutenburg may have iterated but that’s a good thing. We can take advantage of all their testing and not make the same mistakes.

However you don’t have to get this perfect now. You just have to provide a way that works regardless of how the containers are used so that block authors don’t have to remove your UI and replace it with their own.

What I’m suggesting is very close to the ideas in quanta (depending how you interpret it because it doesn’t deal with containers) and doesn’t change the current UI much.

The grid block toolbar is converted to a quanta toolbar “lite” that follows the block that is selected The key point is that it’s very clear what block is selected and the toolbar is contextual to the selected block. You aren’t showing a toolbar for the grid while you are working on a sub-block.

The toolbar doesn’t need slate merged in with yet or a context menu. Those can be done later. It doesn’t need the settings button as selecting the block will automatically do that. It just needs an add and delete and the “add” will always “add after” the current selection so the user experience is consistent.

selectgrid selecttext selectcol

Because this is WYSIWYG and because containers might not have much padding to allow them to be clicked on superate from their contents there needs to be at least one other way of selecting a parent other than clicking. Gutenburg has 4 or 5 ways. We only need one to begin with. Again it doesn’t have to be perfect now.

You didn’t like having breadcrumbs in the side bar even though technically there is no reason it can’t be there but how about this instead?

Screen Shot 2023-06-27 at 8 30 36 pm

but the gutenberg solution at the bottom of the screen is also good and could be added later (so can it’s outline view)

Screen Shot 2023-06-27 at 8 32 36 pm

If you think the method to get to the parent is still too subtle another idea is the quanta toolbar could include a button that moves the selection to the parent. A kind of “back to parent” button. I’ve just noticed volto-columns-block has exactly this. So for grid scenarios you get something like this (not sure on the icon yet).

selectcol_up

The add inserts a new column in the middle and “back” will select the parent grid block so you can adjust its settings.

You will probably say the add button doesn’t make it clear where the insert will take place even if it is consistent and I agree with you. But there is no reason another overlay can’t be used to show where it will be added if desired.

selectcol_up_pos

And clicking “back” we get to the grid block and can then add a new block to the page.

selectgrid_up_pos

As you may have realised, the page is now a container so there is no inconsistency for how blocks work within the page vs how they work inside containers.