Duplicate panel feature#61367
Conversation
…el in the container. Added a naming system for duplicates
src/plugins/dashboard/public/actions/duplicate_panel_action.tsx
Outdated
Show resolved
Hide resolved
src/plugins/dashboard/public/actions/duplicate_panel_action.tsx
Outdated
Show resolved
Hide resolved
…neric (dashboard specific)
…gun writing tests
55d45ad to
69753b2
Compare
|
Nice work! Just a couple of quick takes:
|
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
|
Yes, we prefer "clone" over "duplicate". So the menu item should be "Clone panel" For the notification: Cloned panel |
|
Having a look at this, as Ryan pointed out, positioning of the panels doesn't look good. On the sample flight dashboard, the panels are weirdly rearranged, but we can work on figuring out the best positioning of the cloned widget. Here's what I think the flow should be:
Example, compare the flow above to this flow:
|
…one is done. Reorganized duplication and placement mechanics. TODO: Hide settings on placeholder panel, change placement method to work better
|
Thanks for the idea Maja, I was able to give much quicker feedback to the user by immediately creating and placing a 'placeholder' embeddable in the final spot, then swapping it out for the final embeddable once the clone has finished.
Panel placement definitely needs to be revisited, so I will see if I can make it more robust! |
majagrubic
left a comment
There was a problem hiding this comment.
Looks dope, will have another pass at testing / have a look at the code first thing tomorrow morning 🙌
…space in 3 directions around the panel they're cloned from, failing that, they take the space directly below their origin and push down all panels that conflict
…isn't room push down all panels that are below it.
There was a problem hiding this comment.
I tested this in Chrome 80 on Mac OS and it works great. I've tried:
- duplicating the panel
- duplicating the panel multiple times
- editing one visualization doesn't affect the duplicated one
- filtering works on all cloned visualizations
- changing the title of one of the duplicated panels didn't affect the other panels
- replacing one of the duplicated panels didn't affect the other panels
- maximizing the panel works as expected
One minor nit I found is that in the visualize listing the visualization with the copy title will be placed after copy1, copy2 etc:
If I'm not mistaken, adding a space at the end of copy title will solve this problem.
The placeholder panel is an amazing addition and the user experience is so much nicer now.
In terms of positioning, I think there are still some tweaks to be done, but I would suggest we do them in a subsequent PR, before this becomes too large to merge.
Here is an example: when there are only two panels on the dashboard, duplicating the rightmost one renders the panel immediately below:
In my opinion, this should be rendered in the next row leftmost.
Moreover, if you duplicate this panel now, it will be rendered to the left:
so we basically created this right-to-left flow, which I'm not particularly sure I like.
My suggestion is that after this is merged, you and @ryankeairns make a detailed pass through all the common scenarios of where the duplicated panel is to be placed, and then we can implement it properly.
As I said - to me this is good enough to merge for now, unless someone has strong objections otherwise. Some code comments below.
src/plugins/dashboard/public/application/actions/clone_panel_action.tsx
Outdated
Show resolved
Hide resolved
| defaultMessage: 'copy', | ||
| }); | ||
| const cloneRegex = new RegExp(`\\(${clonedTag}\\)`, 'g'); | ||
| const cloneNumberRegex = new RegExp(`\\(${clonedTag} [0-9]+\\)`, 'g'); |
There was a problem hiding this comment.
What exactly is the purpose of these regexes?
There was a problem hiding this comment.
They are used to remove any cloning artefacts from the name of the savedObject, so that you don't end up with a title like "Hello World (copy) (copy 1) (copy 2)"
| : baseTitle + ` (${clonedTag} ${similarBaseTitlesCount})`; | ||
| } | ||
|
|
||
| private async cloneEmbeddable( |
There was a problem hiding this comment.
Would this be better suited to live on the embeddable itself? In case we have other uses-cases like this? Just thinking out loud, and definitely doesn't need to be made as part of this PR.
There was a problem hiding this comment.
This method currently seems to work on every different type of embeddable, it would be pretty cool though if embeddables had a way to override it.
Additionally, it could later be moved onto the dashboard embeddable so that it could be re-used if we do need it!
| return createPanelState(panelState, this.input.panels); | ||
| } | ||
|
|
||
| public showPlaceholderUntil<TPlacementMethodArgs extends IPanelPlacementArgs>( |
| } from '../../../embeddable_plugin'; | ||
| import { PlaceholderEmbeddable, PLACEHOLDER_EMBEDDABLE } from './placeholder_embeddable'; | ||
|
|
||
| export class PlaceholderEmbeddableFactory implements EmbeddableFactoryDefinition { |
There was a problem hiding this comment.
Do we need a unit test for this?
| const panel = dashboard.getInput().panels[embeddable.id] as DashboardPanelState; | ||
| const action = new ClonePanelAction(coreStart); | ||
| // @ts-ignore | ||
| const newPanel = await action.cloneEmbeddable(panel, embeddable.type); |
There was a problem hiding this comment.
What was the ts-error here?
There was a problem hiding this comment.
the cloneEmbeddable method is set to private
| ); | ||
| // @ts-ignore | ||
| expect(await action.getUniqueTitle('testSecondTitle (copy 20)', embeddable.type)).toEqual( | ||
| 'testSecondTitle (copy 40)' |
There was a problem hiding this comment.
Not sure I understand this - shouldn't the next title be copy 41?
There was a problem hiding this comment.
The titling uses a system like file copies in windows, the order looks like this:
OG Title
OG Title (copy)
OG Title (copy 1)
That means that the next title is always the number of similar titles - 1. So if there were 41 similar titles, the next title would be OG Title (Copy 40)
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Added a new cloning feature for panels on a dashboard.
Summary
Adds the ability to duplicate any embeddable on a dashboard.
Closes #60921
Functionality Overview
The clone functionality does the following when the user executes the action:
This functionality should work on all kinds of embeddables.
Placement
When duplicating, the system creates a new gridData of the correct shape, and uses it to check for free space to the right, to the bottom, and to the left of the cloned panel.
Naming
When a saved object is duplicated, a unique name is created by attaching an internationalized tag, and a count of other savedObjects with a similar title. If the original saved object is called 'testObj', the first duplicate will be called 'testObj (copy)' and the second duplicate will be called 'testObj (copy 1)'
Checklist
Delete any items that are not applicable to this PR.
This was checked for keyboard-only and screenreader accessibilityFor maintainers