Hello. I have not used the Layout Builder Browser module beyond possibly testing it out a few years ago, so I'm not sure exactly how similar the modules are. The intention behind the LB Direct Add module is to bypass unnecessary clicks to start creating block content. The core version requires a click on a "Add block" button and then an additional click to get to the list of block content types. This workflow speeds things up slightly by showing the list of block content types without having to even open the sidebar first.
It looks like the LB Browser module doesn't offer a faster workflow, but instead a richer experience for picking the blocks.
This has been tested by multiple members of our team.
This update was included in https://www.drupal.org/project/gpa_calculator/issues/2682477 📌 GPA Calculator Javascript Cleanup Needs review
Adding contributor from https://www.drupal.org/project/gpa_calculator/issues/2892830 🐛 Grade of "F" not allowed Needs review
Crediting the folks who opened the duplicate issues.
This is a duplicate of https://www.drupal.org/project/gpa_calculator/issues/2682477 📌 GPA Calculator Javascript Cleanup Needs review
Duplicate of https://www.drupal.org/project/gpa_calculator/issues/2682477#comment-156... 📌 GPA Calculator Javascript Cleanup Needs review
Some of this was addressed in https://www.drupal.org/project/gpa_calculator/issues/3459970 📌 10.x+ compatibility Fixed
These changes were addressed in https://www.drupal.org/project/gpa_calculator/issues/3459970 📌 10.x+ compatibility Fixed
Just checked the basic functionality after doing some code updates for code style + deprecations. Works well enough for a dev version.
pyrello → changed the visibility of the branch 3459970-10.x-compatibility to active.
pyrello → changed the visibility of the branch 3459970-10.x-compatibility to hidden.
Also please note, out of an interest for security, we will not ever use the Merge Request for our patching on our sites unless absolutely necessary.
The new best practice workflow for this is to do all issue work in merge requests and then to create/download a patch file from the MR and include it as part of your project repo rather than posting it in an issue comment. There are no security concerns with this approach.
Hello. I'm interested in becoming a maintainer for this module. My employer, the University of Iowa, uses the Drupal 7 version of the module for a site that we are in the process of moving to Drupal 10+. We intend to port this functionality as well.
You'll see from my profile that I have previous experience maintaining modules and have a few modules that I maintain that opt into security coverage.
+1 for theme setting.
pyrello → created an issue.
@naveenvalecha I am currently a maintainer of multiple modules and have a history of contribution as can be seen here: https://www.drupal.org/u/pyrello/issue-credits →
Are you wanting me specifically to demonstrate interest in this module by getting involved with it? I've been working with @bspeare on getting his D10 version of the module put into an MR so that it could be merged.
If you are not interested in helping with the D10+ version, no worries, just trying to help out.
@naveenvalecha I'm a colleague of Ben and interested in helping co-maintain the new branch as well.
@Wim Leers I think that it was marked as "Needs review" so that a person with a higher level of decision-making authority could weigh in on whether an update hook is needed.
This relates to CKEditor 4, so closing this since it never got any traction anyways.
@FiNeX FWIW, we've been using this in production for a while now and I would consider it safe.
@Liam Moreland Good to know. Not sure I would have figured that out on my own and I have yet to discover the docs indicating the proper workflow with MRs yet.
pyrello → changed the visibility of the branch 10.3.x to hidden.
pyrello → changed the visibility of the branch 11.x to hidden.
pyrello → changed the visibility of the branch 10.1.x to hidden.
Setting back to "needs review" because I have a question for @larowlan about the proposed trait that this was set to needs work for.
@Simon Georges Do you think that this would fit your use case? https://www.drupal.org/project/lb_direct_add/issues/3415713 ✨ Add permission to view "More options" link Needs review
@jayhuskins Thanks for submitting the MR! I'm not opposed to this idea. I'll take a look and see how it works.
@nicross-com Looks like the build step was omitted so the plugin got built in non-production mode.
I opened a separate issue to address the broader issues here in a way that won't just reintroduce a random test failures: https://www.drupal.org/project/drupal/issues/3411496 🐛 Re-work off-canvas javascript to fix the UI and prevent random failures Active
@bwaindwain I think the whole reason the original change was made was because of random test failures, so I think it makes sense that the test fails again with the patch reverting that change: https://www.drupal.org/project/drupal/issues/3350972 🐛 [random test failure] Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderUiTest::testReloadWithNoSections() Fixed .
Looking through that original issue, it seems like either state causes problems. In all likelihood this means that the JS needs to be revamped to fix this in a way that won't cause periodic failures (like in the test) or the behavior described in this issue.
@Alina Basarabeanu FWIW, we have started copying patches into our project repo rather than pulling them from drupal.org or the Drupal Gitlab instance. That avoids having to make a copy of the MR diff and post it into the comments of an issue, which ultimately creates more noise for folks that are trying to parse through an issue and make sense of what they should use for a patch.
pyrello → changed the visibility of the branch 3208766-d10_1-key-sections-by-uuid-rebased to hidden.
I suspect that this still needs work. There are some issues where we worked around how Editor Advanced Link works that would be nice to find a better way to resolve in both modules. However, I think this is ready for some testing from anyone who is able to do so.
@laura.gates, I think that should be its own bug report issue.
After a few days of having no idea what I was doing, it started to make sense to me today. Still hoping some CKEditor 5 expert will swoop in and finish it for me ;)
The main things that are left:
- Figure out how to put the button in the correct place.
- Figure out how to take the data from the dialog and insert it into the link field.
So sorry, I also got confused about this. I have been working on D9 upgrades, but this was not related to that. This was definitely on a D7 site.
@jvogt The MR is still in progress and not functional yet. It's my first CKEditor 5 plugin so I'm figuring it out as I go.
The errors you reported may be helpful later! Thanks.
I think this should actually be targeting the dev version.
Going to take a look at this.
Added a draft change record for whenever this get past the finish line.
@crutch - just a small point of clarification: 11.x is essentially the current main branch all development. Since the current cycle is still targeting 10.x releases, those get created from the 11.x branch.
https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... →
I'm just realizing I set this to a different version of the module than I intended to. It looks like the issue is present in both 7x and 8+x versions.
Just ran into the need for this. Added an explicit access check, which is required in Drupal 10. Patch appears to work.
I noticed the code quality warnings in the MR. Nice! I fixed the ones I could figure out off the top of my head how to fix. The others I'm not sure how to deal with. While we are not removing the $additional property, are we doing anything to convert the incoming attributes to third party settings? Need someone who understands this better to weigh in.
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?
I would say that the problem is that it is possible to call mail() without an existing implementation and it still works to send mail. Moreover it's been this way for a long time so my guess is that we are not the only ones that used it like this. It seems like this case should be covered even though it's not standard.
Take a look at where the test I added above fails: https://git.drupalcode.org/project/symfony_mailer/-/blob/1.x/src/MailMan...
The $builder->fromArray() method is called without a check to ensure that $builder actually got set to something. At a minimum, there should be a check here that allows it to gracefully fail or trigger an exception. I would contend instead that the EmailBuilderManager::createInstanceFromMessage() method should return the LegacyEmailBuilder if no other match is found.
I'm happy to add that to my patch, but I was hoping to get some indication of whether this was the correct way to make sure that the LegacyEmailBuilder is used if nothing else can be found.
@AdamPS, I have added a failing test to demonstrate what I'm talking about.
The test doesn't implement hook_mail(). Instead it just calls the mail() method directly. This case doesn't seem to be covered by this module.
I could add a hook_mail() implementation to our test module. However, that also changes the test. Because it is technically possible to call the mai() method directly, the test would no longer cover that case.
Nice.
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?
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.
I would love to see improvements like those suggested in #59. We had rolled our own version of the visible drag handle and a cookie to store the width for the user. The resize functionality for the off-canvas sidebar was broken in 9.5.x (and is still broken in the latest 10.1.x versions last I checked) and we were investigating reverting back to a static width as part of moving to 9.5. That is why I produced the MR that I did. We ultimately decided to keep our enhancements and just deal with the fact that the resize functionality is wonky. Looking forward to seeing the new version. If I get a chance I can try putting together something based on our custom functionality and/or Gin LB.
I'm using https://github.com/joachim-n/drupal-core-development-project and was attempting to run a fresh installation of it today. Installation is broken: https://github.com/joachim-n/drupal-core-development-project. I'm wondering if it might be related to this update?
Fixed failing tests.
Updated IS
Learning curve for adding the deprecation notice :)
@Luke.Leber thanks for pointing that out. I haven't had a chance to dig into the Gin LB implementation yet, but it looks similar to the core's (mostly hidden) off-canvas resizing capabilities. The main difference seems to be that in Gin LB they have the tray on top of the canvas, not next to it.
So, I went the route of making this a piece of configuration. There is no Admin UI yet for changing it. The benefit of this method is that to change the setting for all relevant links, you just need to update the configuration.
I'm sure there's things that I missed and there are no tests for the updates yet, but I set to "Needs review" so that someone can tell me what is missing and what should be tested.
pyrello → made their first commit to this issue’s fork.
I think I fixed the error in the test that was failing, but I didn't really understand what I was doing.
Updated issue summary.
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.
Added a test to cover adding, updating, and removing blocks.