Skip to content

[DS-1327] Add the Layout builder copy/paste section module #215

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[DS-1327] Add the Layout builder copy/paste section module #215

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f8b6e0
[DS-1327] Add the Layout builder copy/paste section module
Apr 19, 2024
84d2a24
[DS-1327] Add fix for styles for the Clone section functionality
May 13, 2024
7195418
[DS-1327] Remove lb_copy_section module from composer
May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"drupal/lb_accordion" : "^2.1.0",
"drupal/lb_cards" : "^2.0.4",
"drupal/lb_carousel" : "^2.0.3",
"drupal/lb_copy_section" : "^1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about upgrade path?
We need to add this module to standard|extended profile variation and enable it via hook_update_N @aleevas

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @podarok, we think this might overload the interface for some YMCAs who might not need it, that's why we made it optional, but if it's necessary then so be it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @podarok. If we're not installing and configuring this module, then we should not include it in composer and simply document the installation and use. I'm checking in with Shelley to discuss the best path forward.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, waiting for the decision.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rollki I've discussed with Shelley and also tested the module and I think it's ok to go ahead with, but we need to test and ensure it works well before committing.

I tested and it seems there are some issues with cloning. My test steps:

  • go to the homepage
  • clone the body section
  • edit the cloned block
  • BUG 1: Nested entity references are not editable.
  • BUG 2: Creating a new sub-block and attempting to save results in the error "This entity could not be referenced"
  • BUG 3: "Copy" text is larger than "Configure"
SCR-20240423-jpwe SCR-20240423-joub SCR-20240423-jqom

Copy link
Contributor Author

@aleevas aleevas May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@froboy the issue with cloned blocks should be fixed in this PR YCloudYUSA/y_lb_demo_content#45

The Copy Layout Builder was added to the OpenY profile YCloudYUSA/yusaopeny#188
And this module was removed from composer of the Y LB module 7195418d50986862c0431a37f2e37683631d0f8f

Copy link

@rollki rollki May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think this solves the current problems even though it affects a few modules. There is probably no simple solution.

But now since we make blocks for LB reusable at YCloudYUSA/y_lb_demo_content#45 it means that these cloned blocks will appear in the "All system blocks" section when adding a block to LB, there mainly system blocks and probably few people use these blocks, but here is the question: will it be ok for us?

сс @froboy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rollki I strongly disagree with this approach.

as you say:

it means that these cloned blocks will appear in the "All system blocks" section when adding a block to LB...

This means that dozens or hundreds of blocks will be listed in "All system blocks". This is a very bad idea and will cause chaos for content editors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above:

I ran into this on another project and had to add two different patches and make some changes in how entity references work. It will probably take more work to make complete.
https://www.drupal.org/node/3098245
https://www.drupal.org/project/inline_entity_form/issues/3002175

We'll probably need some patches to get this to work (which will resolve other issues related to cloning) if we still want to proceed down this path. I'm not sure though.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that making reusable blocks is not a solution considering that new blocks may appear and it will be necessary to keep in mind that they must also be reusable + that there will be too many inline blocks.

In this case, we need to try to apply these patches, but there is a high probability that they will need to be modified and updated. сс @aleevas

@froboy please confirm if this sounds good to you. Also, PR YCloudYUSA/y_lb_demo_content#45 is probably not needed.

Note: Patching may take some time, which means this feature probably won't make it to the current release.

"drupal/lb_grid_cta" : "^3.0.4",
"drupal/lb_hero" : "^1.2.0",
"drupal/lb_modal" : "^1.2.1",
Expand Down