Closed (fixed)
Project:
Experience Builder
Version:
0.x-dev
Component:
Page builder
Priority:
Major
Category:
Feature request
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
19 Jul 2024 at 11:01 UTC
Updated:
15 Nov 2024 at 22:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
wim leersIf the entire JSON structure is copied, I don't see how this could fail? 🤔
Comment #3
lauriiiThis might be something we should give further consideration but I would imagine users would not expect that copying a component also copies the property expressions source data. This means that copying a component to a new host entity could lead into situation where property expression could be pointing to a target which doesn't exist in the new host entity.
Comment #4
wim leersAh, yes.
StaticPropSources would copy over just fine, but notDynamicPropSources — for these, users should indeed be prompted.That's also what the
DynamicPropSoruce::$expressionis for:\Drupal\experience_builder\PropExpressions\StructuredData\StructuredDataPropExpressionInterface::isSupported()literally is designed to allow detecting this 👍IOW: the back end already has the necessary infrastructure.
Comment #5
lauriiiAdding this to the board as a potential delighter to add.
Comment #6
freelockThis would be huge -- and I see it as the single biggest advantage of Gutenberg over other Drupal solutions...
Comment #7
wim leersComment #8
utkarsh_33 commentedComment #10
utkarsh_33 commentedThis is not a completely working MR thought but it duplicates the nodes and place it next to the selected node for now using the
duplicate node reducersdefined inlayoutModelSlice.I have a bunch of questions here:-
Ctrl+Cwe will get the data related to that component insidecopyComponentreducer.With the data copied along with the component to copy how do we define the place where we want to paste the component?For now we are using theduplicateNodereducers which places this next to the copied component.One probable solution that i can think of (might not be a good solution though) is to have spaces around the components in certain shapes which has a+ Iconto add a component at that place.Because currently we cannot insert a component at desired location without dragging a component.Comment #11
wim leersalert('This component has no slots.');alert('This component has no slots.');That won't be the final UX obviously (since there are no designs for this), but that's super MVP to be able to have a more detailed conversation!
Comment #12
wim leersComment #13
utkarsh_33 commentedSo the current state of the MR does the following:-
Ctrl+Cthen it copies the component and if we pressCtrl+Vthen it pastes the component next to the selected component just like duplicate node does.Ctrl+Cthen it copies the component and then we select another component after which we want to insert the component and the pressCtrl+Vthen it appends the copied component after the selected component.Comment #14
utkarsh_33 commentedI was trying to write a test case for this which is as follows:-
I figured out that cypress tests does not support copy paste.You can see https://github.com/cypress-io/cypress/issues/2386 for more details.If anyone knows a better way to trigger copy paste with keyboard that will be really helpful.
Comment #15
utkarsh_33 commentedLeft some questions that i need clarification with.I'm not sure who is the right person to answer those but marking it NR.
Comment #17
ctrladelTook a look at this at the Barcelona contrib day. Found and fixed a bug with the logic for the keyboard shortcuts so copy/paste actually works now.
I don't think reusing duplicateNode is going to be the right approach or at least it does not fully cover all the needed functionality. To be able to copy components between different nodes the component needs to be placed on the system clipboard and the json inserted into the tree. With the current approach it looks like we're only copying/pasting the component from app state.
Comment #18
jessebaker commentedAs @ctrladel says in #17 I think the implementation here is more like duplicate with an extra step. It does not allow a user to, for example, copy a component, reload the page to remove other unwanted changes and then paste the component back in again. Or take a component from one node and paste it onto another.
I think perhaps some of this work can be repurposed to make a CMD+D shortcut to allow for instant duplication of the selected component.
Comment #19
utkarsh_33 commentedComment #21
bnjmnmPushed the MR in a very WIP state so I'm switching it to draft- needs some cleaning up that I don't have time for atm, but it now works like a true copy paste. The original can be deleted but still pasted, and pasting can happen from one tab to the next.
It is using localStorage, not clipboard, since clipboard only works with https and it's not uncommon to work on a non-https local. If we really want clipboard support I recommend doing it in a separate issue as we will need to determine if it is worth having two copypaste apis to support, and the possible confusion that might occur from pasting a stringified layout / model object into a document.
Comment #22
lauriiiThe
localStorageidea seems like a smart way to implement this so long as it works between tabs. 😊This is not in the scope of this issue but it would be great to be eventually able to copy and paste to XB from other apps. This could mean for example inserting text into a text element. I was wondering how would we differentiate between the state and the clipboard. Maybe we could achieve this by clearing the clipboard whenever we store XB clipboard in state. This way we could first check if the clipboard has content, and use it instead when that's the case.
Should we handle CTRL+X here or move that too a follow-up?
Comment #23
bnjmnmThe tests now work - not quite ready to set to NR as we should test scenarios like pasting a component that has been deleted and if possible, pasting between tabs.
Comment #24
bnjmnmComment #25
saurav-drupal-dev commentedplease add steps to reproduce how can i test it and where?
Comment #26
hooroomooThere should also be copy and paste functionality added to the layers menu in a follow-up
Comment #27
hooroomooOne comment
Comment #28
jessebaker commented@bnjmnm is really busy with a bunch of other things at the moment. So if anyone else wants to push forward on this (implementing @hooroomoo's suggestion above) please feel free to take it on!
Comment #29
hooroomooComment #30
hooroomooCurrently the issue is in the MR if a user copies and pastes within the iframe they can't do it consecutively because the it seems like the iframe doesn't receive the key events after the first time. So either the iframe would need to get focused again or the parent element outside the iframe needs the key listeners for copy and paste.
But since #3475759: Implement a different approach to the iFrame preview interactions gets rid of cross communication between iframe and the parent, I kinda want to wait till that's in since the way this is currently implemented will need to change anyway.
Comment #31
jessebaker commented#3475759: Implement a different approach to the iFrame preview interactions has landed!
Comment #32
hooroomooUn-assigning myself for now in case anyone else wants to pick it up, get changes from 0.x first
Comment #33
jessebaker commentedI went ahead and resolved the conflicts on merging 0.x as it was quite tricket. Doing that has broken the new Cypress test because that still interacts with the iFrame in the 'old' way.
I think going through and updating the Cypress test to replace e.g.
cy.getIframeBody().findAllByText('XB Needs This For The Time Being').should('have.length', 2);withcy.clickComponentInPreview('Hero', 1)will get the test working again.Leaving unassigned if anyone wants to pick up, else I can jump on this tomorrow.
Comment #34
jessebaker commentedComment #36
jessebaker commentedComment #37
jessebaker commentedThis is currently working with one minor visual issue that looks pretty bad. After pasting the overlay/outline UI flashes up, unstyled/broken.
I think this is ready for review, but I will continue looking into this last visual issue. Depending on the scale of the fix, I will either push a further commit to this branch or create a new issue.
Comment #38
hooroomooWorks great! Works between tabs and also from layers menu too. Just a couple of comments
Comment #40
hooroomoo