- Issue created by @lauriii
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This may lead into error scenarios, like for example if the component had property expressions in its properties. In this scenario we should display the property expression as "missing". The user can either fill in it manually, or map it to another property.
If the entire JSON structure is copied, I don't see how this could fail? 🤔
- 🇫🇮Finland lauriii Finland
This 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.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This means that copying a component to a new host entity
Ah, yes.
StaticPropSource
s would copy over just fine, but notDynamicPropSource
s — for these, users should indeed be prompted.That's also what the
DynamicPropSoruce::$expression
is for:\Drupal\experience_builder\PropExpressions\StructuredData\StructuredDataPropExpressionInterface::isSupported()
literally is designed to allow detecting this 👍IOW: the back end already has the necessary infrastructure.
- 🇫🇮Finland lauriii Finland
Adding this to the board as a potential delighter to add.
- 🇺🇸United States freelock Seattle
This would be huge -- and I see it as the single biggest advantage of Gutenberg over other Drupal solutions...
- Merge request !330#3462633 : Allow copy pasting components with CTRL+C and CTRL+V. → (Merged) created by utkarsh_33
This 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 reducers
defined inlayoutModelSlice
.
I have a bunch of questions here:-
1) Say we select the component and copy that component with the use ofCtrl+C
we will get the data related to that component insidecopyComponent
reducer.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 theduplicateNode
reducers 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+ Icon
to add a component at that place.Because currently we cannot insert a component at desired location without dragging a component.
2) Also i wanted a bit clarification of what all we need to copy.IMHO i think it makes sense to duplicate the whole node as we do in the duplicate node scenario and then let the user alter the props according to their needs.- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
- Very good question. Let's start with the simpler UX. Let's only allow CTRL+V when selecting a component or slot in the "layers" panel. Then:
- if a slot is selected, append to the slot
- if a component is selected, append to its first slot — and if there are no slots,
alert('This component has no slots.');
- if nothing is selected,
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!
- +1! 😊
- Very good question. Let's start with the simpler UX. Let's only allow CTRL+V when selecting a component or slot in the "layers" panel. Then:
So the current state of the MR does the following:-
- If we select a component and press
Ctrl+C
then it copies the component and if we pressCtrl+V
then it pastes the component next to the selected component just like duplicate node does. - If we select a component and press
Ctrl+C
then it copies the component and then we select another component after which we want to insert the component and the pressCtrl+V
then it appends the copied component after the selected component.
- If we select a component and press
I was trying to write a test case for this which is as follows:-
it('Copy and paste a node using keyboard shortcuts', () => { cy.loadURLandWaitForXBLoaded(); cy.getIframeBody() .find('[data-xb-uuid="dynamic-image-udf7d"]') .first() .click({ force: true }); cy.get("body").type("{ctrl}C").wait(100); cy.getIframeBody() .find('[data-component-id="experience_builder:my-hero"]') .first() .click({ force: true }); cy.get("body").type("{ctrl}V").wait(100); cy.pause() // Assert the position of the copied element. });
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.
Left some questions that i need clarification with.I'm not sure who is the right person to answer those but marking it NR.
- First commit to issue fork.
- 🇺🇸United States ctrladel North Carolina, USA
Took 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.
- 🇬🇧United Kingdom jessebaker
As @ctrladel says in #1 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.
- First commit to issue fork.
- 🇺🇸United States bnjmnm Ann Arbor, MI
Pushed 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.
- 🇫🇮Finland lauriii Finland
The
localStorage
idea 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?
- 🇺🇸United States bnjmnm Ann Arbor, MI
The 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.
- 🇺🇸United States hooroomoo
There should also be copy and paste functionality added to the layers menu in a follow-up
- 🇬🇧United Kingdom jessebaker
@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!
- 🇺🇸United States hooroomoo
Currently 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 🌱 [Proposal] A different approach to the iFrame preview interactions Active 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.
- 🇬🇧United Kingdom jessebaker
🌱 [Proposal] A different approach to the iFrame preview interactions Active has landed!
- 🇺🇸United States hooroomoo
Un-assigning myself for now in case anyone else wants to pick it up, get changes from 0.x first
- 🇬🇧United Kingdom jessebaker
I 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.
- 🇬🇧United Kingdom jessebaker
This 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.
- 🇺🇸United States hooroomoo
Works great! Works between tabs and also from layers menu too. Just a couple of comments
-
hooroomoo →
committed 087041bc on 0.x authored by
utkarsh_33 →
Issue #3462633 by jessebaker, hooroomoo, bnjmnm, hemanshu_412: Allow...
-
hooroomoo →
committed 087041bc on 0.x authored by
utkarsh_33 →
- Status changed to Fixed
about 1 month ago 10:24pm 15 November 2024 Automatically closed - issue fixed for 2 weeks with no activity.