[IMP] clipboard: paste conditional format, data validation & merge content when original content is deleted #4170
Conversation
Rachico
force-pushed
the
master-keep-cf-and-merge-at-paste-mera
branch
2 times, most recently
from
852969d
to
115f8e8
Compare
Rachico
force-pushed
the
master-keep-cf-and-merge-at-paste-mera
branch
5 times, most recently
from
625d9a5
to
e279183
Compare
|
-> change the title as a [FIX] : the rationale is the following - we currently lose information when copy/pasting and the current result is different from the intended one. |
Rachico
force-pushed
the
master-keep-cf-and-merge-at-paste-mera
branch
from
e279183
to
40ba85f
Compare
| target: Zone[], | ||
| content: T1[][], | ||
| options?: ClipboardOptions, | ||
| clipboardContent?: T |
There was a problem hiding this comment.
Are you sure you need to add a second key clipboardContent?
Wouldn't it be possible to just add more information in the content key just like you did in the merge clipboard?
There was a problem hiding this comment.
After discussing with Anthony:
- the typing of T1 was set to be the minimal required information to manage the pasting
- We can change it to whatever we want. ->
content: Tis an acceptable change
| paste(target: ClipboardPasteTarget, clippedContent: T, options: ClipboardOptions | undefined) {} | ||
| paste( | ||
| target: ClipboardPasteTarget, | ||
| clippedContent: T | MinimalClipboardData, |
There was a problem hiding this comment.
T is a generic (https://www.typescriptlang.org/docs/handbook/2/generics.html), so it could be a MinimalClipboardData.
If you are certain this should be a minimalClipboardData (or undefined, as it can currently be the case), then it might be a plan to update it like you did for the isPastAllowed below.
However, I would discuss with @anhe-odoo first to see if he had a reason to use a generic here.
There was a problem hiding this comment.
After discussing with Anthony:
clippedContent was set as a generic such that every handler could define their own type and only deal with the data they wanted/cared about. It is a good approach and should stay that way.
On the other hand, nothing prevents us from having the type of the handler extending an interface representing the global keys found in the clipboard (more something like ClipboardCellData rahter that minimalClipboardData if possible).
Rachico
force-pushed
the
master-keep-cf-and-merge-at-paste-mera
branch
8 times, most recently
from
ecb018d
to
e129216
Compare
Rachico
changed the title
Rachico
force-pushed
the
master-keep-cf-and-merge-at-paste-mera
branch
from
e129216
to
ce7d734
Compare
| this.pasteFromCopy( | ||
| sheetId, | ||
| zones, | ||
| clippedContent.cfRules as ClipboardConditionalFormat[][], |
There was a problem hiding this comment.
this cast looks wrong no ? clippedContent.cfRules can definetly have undefined values
| this.pasteFromCopy( | ||
| sheetId, | ||
| zones, | ||
| clippedContent.dataValidationRules as ClipboardDataValidationRule[][] |
There was a problem hiding this comment.
Same comment than for CFs
| return; | ||
| } | ||
| const sheetId = this.getters.getActiveSheetId(); | ||
| this.pasteFromCopy(sheetId, target.zones, content.cells, options); | ||
| this.pasteFromCopy(sheetId, target.zones, content.merges as Merge[][], options); |
There was a problem hiding this comment.
Same comment. As a rule of thumb, if you have to cast something 90% of the times there's something wrong somewhere.
| if (!content.cells) { | ||
| return CommandResult.Success; | ||
| } | ||
| const clipboardHeight = content.cells.length; | ||
| const clipboardWidth = content.cells[0].length; |
There was a problem hiding this comment.
We can replace content.cells by content.merges now no ? That way we don't reference cells, chich is introduced by another clipboard
| @@ -1547,6 +1606,137 @@ describe("clipboard", () => { | |||
| expect(getStyle(model, "C2")).toEqual({}); | |||
| }); | |||
|
|
|||
| test("copy cells with CF => remove origin CF => paste => it should paste with original CF", () => { | |||
There was a problem hiding this comment.
In my opinion, we should aim to make tests short and readable, rather than try to test absolutely everything. I don't think you need to test that every single command is successfully dispatched, nor do you need to test both than the cell content is pasted AND the CF is pasted AND that the Cf succefully is applied. That's a lot of bloating, making the test hard to understand at first glance IMO.
The test could be 15 lines instead of 60, only adding a CF => copy it => Remove the CF => paste => check the CF is pasted. We don't need to also test that we can paste cell values, nor that CF are correctly applied to the cells. It's tested somewhere else.
2 small general details:
- try to prettify thing on single lines when you can.
expect(getStyle(model, "A1")).toEqual({fillColor: "#00FF00" });is short enough to be on a single line, no need to take 3 lines - if you want to test that a command behaved correctly, avoid testing
toBeSuccessfullyDispatchedbut rather the state of the model after the command. For example hereREMOVE_CONDITIONAL_FORMATwould have been dispatched, but not actually removed the CF. But again, you don't need to test that every single command is successful, I'd say to only test it if it helps the readability of the test.
There was a problem hiding this comment.
(same comment for the next test)
Rachico
force-pushed
the
master-keep-cf-and-merge-at-paste-mera
branch
3 times, most recently
from
0c64a7e
to
b271631
Compare
Rachico
force-pushed
the
master-keep-cf-and-merge-at-paste-mera
branch
2 times, most recently
from
b86f3e1
to
6575968
Compare
…ntent when original content is deleted Currently, when copy/pasting cells with CF/DV/merge, if we delete the CF/DV/merge before pasting the cells, the paste content will not contain the previously copied content. This commit solves the issue by adding new attributes cfRules, merges & dataValidationRules in the clipboard content and using the content saved in these keys to re-create the features. Task: 3597039
Rachico
force-pushed
the
master-keep-cf-and-merge-at-paste-mera
branch
from
6575968
to
50c3d9d
Compare
[IMP] clipboard: paste conditional format, data validation & merge content when original content is deleted
Problem
Before this commit, when copy/pasting cells with CF/DV/merge, if we delete the CF/DV/merge before pasting the cells, the paste content will not contain the previously copied content.
Solution
This commit solves the issue by adding new attributes cfRules, merges, dataValidationRules in the clipboard content and using the content saved in this key to re-create the features.
Task: 3870289
review checklist