- last update
over 1 year ago 29,418 pass - Merge request !4380Issue #3321255: Only reload updated part of Layout Builder element → (Open) created by pyrello
- last update
about 1 year ago Custom Commands Failed - Status changed to Needs review
about 1 year ago 2:33pm 13 July 2023 - 🇺🇸United States pyrello
Added a test to cover adding, updating, and removing blocks.
- last update
about 1 year ago Unable to generate test groups - last update
about 1 year ago 29,814 pass - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago 29,815 pass - last update
about 1 year ago 29,815 pass - Status changed to Needs work
about 1 year ago 6:28pm 17 July 2023 - 🇺🇸United States smustgrave
Can a CR be created for the new trait?
Think the "Needs tests" can be removed too. Looks like you added those unless there are additional you're eyeing?
- 🇺🇸United States pyrello
I added a draft change record. I've never written one before, so would very much welcome feedback.
I wasn't planning on any additional tests.
- Issue was unassigned.
- Status changed to Needs review
about 1 year ago 10:04pm 17 July 2023 - Status changed to RTBC
about 1 year ago 9:48pm 22 July 2023 - 🇺🇸United States smustgrave
Verified the issue using the browser inspector. I added 2 custom blocks
Edited 1 of the blocks and the entire form dom refreshed.Applied the MR 4380
Verified that layout builder still functions as before. Tested by enabling for Article content type
Adding a custom block
Adding an Authored by field block
Moved some blocks around
Removed the custom block.
Added back.Using the browser inspector and reusing the 2 custom blocks from before
Edited 1 of the blocks and see that just that div refreshed vs the entire form.Fantastic!
- last update
about 1 year ago 29,870 pass, 2 fail - last update
about 1 year ago 29,882 pass - last update
about 1 year ago 29,887 pass - last update
about 1 year ago 29,890 pass 54:22 51:26 Running- last update
about 1 year ago 29,949 pass - last update
about 1 year ago 29,949 pass - last update
about 1 year ago 29,956 pass - last update
about 1 year ago 29,956 pass - last update
about 1 year ago 29,961 pass - last update
about 1 year ago 29,961 pass - last update
about 1 year ago 29,961 pass - last update
about 1 year ago 29,965 pass - last update
about 1 year ago 30,050 pass - last update
about 1 year ago 30,054 pass - 🇫🇮Finland lauriii Finland
Woah, epic work @pyrello!! 👏 Tagging for subsystem maintainer review in case one of the Layout Builder maintainers would like to review this.
- last update
about 1 year ago 30,059 pass - last update
about 1 year ago 30,061 pass - last update
about 1 year ago 30,063 pass - last update
about 1 year ago 30,063 pass - last update
about 1 year ago Build Successful - last update
about 1 year ago 30,138 pass - last update
about 1 year ago 30,138 pass - last update
about 1 year ago 30,139 pass - Open on Drupal.org →Environment: PHP 8.2 & MySQL 8last update
about 1 year ago Waiting for branch to pass - last update
12 months ago 30,149 pass - Status changed to Needs review
12 months ago 4:43am 11 September 2023 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I have a question here. By default LB works against the shared temp store until save is pressed.
In theory that means two people can edit the same layout at the same time.
Currently if someone makes one change, the 'reload everything' nature means that they would see each others changes.
With this change, they would not without a full page reload.
Do we think this is a use-case we need to think about?
Left a minor review on the MR. The tests are very clever, nice one.
- 🇺🇸United States pyrello
In theory that means two people can edit the same layout at the same time.
It had never occurred to me that this was a feature of the reload everything approach.
Do we think this is a use-case we need to think about?
I have no data to back this up, but my guess is that most people have no idea that this is how LB works. I had never realized it and I have worked with LB extensively. Insofar as it is a feature, it is an imperfectly designed one because you are getting the "updates" from the other editor in bursts rather than in realtime. I'm not sure what the right answer is, but it seems like this patch would represent a choice to optimize for perceived performance/responsiveness at the expense of collaborative editing.
To provide a little more context, one of the motivations behind this change is to provide a path forward for potentially having in-place editing for Layout Builder. By providing the ability to update just a single block vs the entire LB object, it is theoretically easier to create such a system.
- 🇺🇸United States smustgrave
Should there be a feature that only 1 user can edit layout builder at a time?
- 🇺🇸United States pyrello
Should there be a feature that only 1 user can edit layout builder at a time?
@smustgrave Can you elaborate on what you mean by that?
- 🇺🇸United States smustgrave
If the concern is around 2 people editing the same page. Could we implement some kind of checkout feature where only 1 user can edit a page layout at a time.
- Status changed to Needs work
11 months ago 7:34pm 12 October 2023 - 🇺🇸United States smustgrave
Moving to NW for the single trait mentioned by @larowlan.
Still not sure about the multiple users editing at once. Not sure if layout builder should support that or if it could be a contrib.
- 🇬🇧United Kingdom catch
If the concern is around 2 people editing the same page. Could we implement some kind of checkout feature where only 1 user can edit a page layout at a time.
We have this for entities (give or take bugs #2892132: Entity validation does not always prevent concurrent editing of pending revisions → ) so it would not be unreasonable to implement it here too.
- 🇺🇸United States pyrello
Here are a couple scenarios I have come up with to try to understand this a bit better. For each, there are two people, person A and person B, who are working in LB for a node at the same time.
Person A edits block X and person B edits block Y. Block X is saved slightly before block Y.
With current functionality:
After person A saves block X, person B will not see the changes to block X reflected in LB until after they have saved block Y. Person A will see the updates to block Y after they save the layout or make another change to a block.With proposed functionality:
After person A saves block X, person B will not see the changes to block X reflected in LB until after they save the layout or try to edit block X.Person A is editing block X and person B also starts editing block X before person A has saved their changes.
With current functionality:
Because person B already has the block form up, they do not see the changes to block X that person A saved. When they save the block, their changes become the new version of the block and their is no message to indicate that they have overridden person A's changes that happened since the last time they saw an up-to-date version of the node. Person A will either see person B's changes when they save the node or when they save changes to another block, or possibly not at all if they save the node prior to person B saving their changes to the block.With proposed functionality:
The experience is the same here except for the case where person A edits another block. At that point they will not see person B's changes to block X because the whole layout doesn't get refreshed.It seems like the simultaneous editing of layout feature has some issues that potentially cause unintended overwriting of content data. There is no warning or error to a user letting them know that the block they are working on or have just saved has changed data that they have never seen. It seems like regardless of whether the proposed functionality for this issue is adopted or not there should be some changes to warn or prevent a user from overwriting data they didn't mean it overwrite. Ideally, we should clarify whether two people are allowed to make layout changes simultaneously or not and make changes that support that decision.
We have this for entities (give or take bugs #2892132: Entity validation does not always prevent concurrent editing of pending revisions) so it would not be unreasonable to implement it here too.
Based on this, it seems like it would make sense to move towards functionality to not allow simultaneous editing of the same layout to be more internally consistent with how editing of entities is supposed to work.
It seems like the proposed functionality slightly exacerbates the disconnect between what two users working on the same node will know of each others changes. Is the degradation enough to warrant pausing this update until after we clarify the intended workflow for multiple users and implement changes to support/enforce that workflow? Or can we accept the imperfections to accept this change and then work on more intentional changes to the workflow afterwards?
- 🇫🇮Finland lauriii Finland
Currently if someone makes one change, the 'reload everything' nature means that they would see each others changes.
With this change, they would not without a full page reload.
Do we think this is a use-case we need to think about?
I do think the behavior change is fine. I've talked to > 50 users about building pages with Layout Builder and Paragraph in the last 5 months and using the current tools for concurrent editing within Drupal hasn't come up in any of the workflows I've heard from them. Editing concurrently is a valid need but at the moment most users are collaborating outside of Drupal because we are not providing sufficient tooling for concurrent editing.
We have a pre-existing issue for introducing the locking: 🐛 Concurrent editing of layouts is very confusing Needs work to prevent concurrent editing (until we can support it properly).
So from that perspective, I think we should be good to move forward with the current proposal.
- First commit to issue fork.
- Status changed to Needs review
7 months ago 4:58pm 5 February 2024 - 🇺🇸United States pyrello
Setting back to "needs review" because I have a question for @larowlan about the proposed trait that this was set to needs work for.
- Status changed to Needs work
7 months ago 10:28am 22 February 2024 The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.