mrc-5443 Copy variable on drag if Ctrl key is pressed #214
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
david-mears-2
approved these changes
Aug 6, 2024
| @@ -19,11 +19,13 @@ export default ( | |||
| const thisSrcGraphConfig = hasHiddenVariables ? "hidden" : graphIndex!.toString(); | |||
|
|
|||
| const startDrag = (evt: DragEvent, variable: string) => { | |||
| const { dataTransfer } = evt; | |||
| const { dataTransfer, ctrlKey, metaKey } = evt; | |||
There was a problem hiding this comment.
I dislike having to think to work out what abbreviations stand for - had to go back a line to decipher whether evt meant event or environment
Comment on lines
97
to
102
| expect(setData.mock.calls[0][0]).toBe("variable"); | ||
| expect(setData.mock.calls[0][1]).toStrictEqual("S"); | ||
| expect(setData.mock.calls[1][0]).toStrictEqual("srcGraphConfig"); | ||
| expect(setData.mock.calls[1][1]).toStrictEqual("0"); | ||
| expect(setData.mock.calls[2][0]).toBe("copyVar"); | ||
| expect(setData.mock.calls[2][1]).toBe("false"); |
There was a problem hiding this comment.
Suggested change
| expect(setData.mock.calls[0][0]).toBe("variable"); | |
| expect(setData.mock.calls[0][1]).toStrictEqual("S"); | |
| expect(setData.mock.calls[1][0]).toStrictEqual("srcGraphConfig"); | |
| expect(setData.mock.calls[1][1]).toStrictEqual("0"); | |
| expect(setData.mock.calls[2][0]).toBe("copyVar"); | |
| expect(setData.mock.calls[2][1]).toBe("false"); | |
| expect(setData.mock.calls[0].slice(0, 2)).toBe(["variable", "S"]); | |
| expect(setData.mock.calls[1].slice(0, 2)).toBe(["srcGraphConfig", "0"]); | |
| expect(setData.mock.calls[2].slice(0, 2)).toBe(["copyVar", "false"]); |
Easier to read, if it works? Now I can see that copyVar is meant to relate to "false".
There was a problem hiding this comment.
That doesn't quite work, but toHaveBeenNthCalledWith gives us much the same thing, so switching to that.
Comment on lines
157
to
168
| if (s === "copyVar") return "false"; | ||
| return null; | ||
| } | ||
| }; | ||
| const dropPanel = wrapper.find(".graph-config-panel"); | ||
| await dropPanel.trigger("drop", { dataTransfer }); | ||
| expect(mockUpdateSelectedVariables.mock.calls.length).toBe(1); | ||
| expect(mockUpdateSelectedVariables.mock.calls[0][1]).toStrictEqual({ | ||
| graphIndex: 0, | ||
| selectedVariables: ["S", "R", "I"] | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Can we hold down a ctrl/meta key here, to show that trying to copy a hidden variable on dragging doesn't end up with it existing in both places (hiddens and drag target)
M-Kusumgar
approved these changes
Aug 7, 2024
david-mears-2
approved these changes
Aug 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If user presses Ctrl (or ⌘) key when dragging variabe to another graph, this makes a copy so graph is retained in the source as well as being added to target.
Dragging to or from Hidden variables ignore Ctrl key.
Now that we can have variables on multiple graphs, this also means that we have to make good on the text "Drag variables here to hide them on all graphs" on Hidden variables, so we search all graph configs for variable to hide rather than just removing from source.
It's not very clear to me why, but I found I now had to specify start and end positions in the e2e drag variable tests. Perhaps something to do with the text change in the drop area meaning the text wraps and height is increased in the default page size? Very odd!