accordproject / web-components

React Components for Accord Project
Apache License 2.0
120 stars 101 forks source link

fix(ui-contract-editor): prevent clause drag and drop in list - I157 #231

Closed sanketshevkar closed 3 years ago

sanketshevkar commented 3 years ago

…list - #157

Signed-off-by: User shevkar.sanket@gmail.com

Closes #157

Changes

Flags

Screenshots or Video

-Demo Outside the list: https://www.loom.com/share/954e26d39c114443a2057c6a082e9088 -Demo Inside the list: https://www.loom.com/share/0b52426ebd154db8958adf5c7a0822ad

Related Issues

Author Checklist

sanketshevkar commented 3 years ago

I was testing locally, I had some problems with that. CLI suggested npm test -- -u, I guess that updated test snapshot files. Will that cause any issue? After running npm test -- -u all test were passed locally.

Michael-Grover commented 3 years ago

@sanketshevkar Thanks for sharing videos on Loom, that's very helpful. I watched the two demos and was wondering what determines whether a smart clause is placed inside a list vs outside of a list?

@jeromesimeon do you think we should allow clause templates to be able to be placed inside of lists, like in the second video @sanketshevkar shared?

sanketshevkar commented 3 years ago

@Michael-Grover you might have noticed that there is a small pipe cursor with the drag-drop icon when we are performing the drag-drop action. If this cursor is placed outside at the end of the list item it gets placed outside. If it is placed before the first start of the list item, it gets placed inside.

Michael-Grover commented 3 years ago

The technology working group had a discussion about this issue today, and we decided that clause templates should not be able to be placed inside of a list. So the functionality should be like what currently happens when you drag a clause template onto a paragraph.

Currently, when you drag and drop a clause template near the beginning of a paragraph, the clause template is placed before the paragraph. When you drag and drop a clause template near the end of a paragraph, the clause template is placed after the paragraph. The clause template is never placed in the middle of the paragraph.

We would like to to replicate that same behavior with lists.

sanketshevkar commented 3 years ago

@Michael-Grover I'll work on it. @irmerk Is there any way by which we can track what was the position of list item on which we drop the clause, from which we determine whether to place the clause above or below? Currently I'm only placing it above the list.

Michael-Grover commented 3 years ago

@sanketshevkar, @irmerk is out of office so may not be able to answer quickly, however @DianaLease may be able to help. She worked on the drag and drop functionality for placing smart contracts before or after paragraphs.

DianaLease commented 3 years ago

@Michael-Grover I'll work on it. @irmerk Is there any way by which we can track what was the position of list item on which we drop the clause, from which we determine whether to place the clause above or below? Currently I'm only placing it above the list.

Take a look at this piece of code that looks at where the drop target is in the children in relation to the parent node. If it's at a child in the top half, place it above; if it's in the bottom half, place it below. See if you can adapt that same logic for lists :)

sanketshevkar commented 3 years ago

Successful Usecase: Whenever there are nodes above and below the list. It works properly. https://www.loom.com/share/803b8f6eca93483b9b295e1bd4acd0bf

Failed Usecase: Whenever there are no nodes above the list and we aim to push the clause above the list or whenever there are no blocks below the list and we aim to push the blocks below the list. https://www.loom.com/share/65c98fc5b86d4a60b53f937edc232138

Approach: Since the start and the end edge of ul_list or ol_list lies within the list itself, I decided to choose nodes above or below the list using Transform select and add the clause to them.

Problem: Since there are no nodes above or below in the failed usecase, it wasn't working.

@DianaLease is there a way by which we can set the start and the end edge of the list outside the list?

sanketshevkar commented 3 years ago

@DianaLease @Michael-Grover any updates?

DianaLease commented 3 years ago

Hmm, if there are no nodes above the list, can we assume that is the top of the document and put the clause at the very beginning? And same for if there are no nodes below the list but put it at the very end?

sanketshevkar commented 3 years ago

@DianaLease I tried doing that, but it doesn't seem to work. I hardcoded the top node path in transform.select but still it wasn't working. I am thinking of adding an extra empty node at the top and the bottom of the editor, will this be the right approach?

DianaLease commented 3 years ago

@DianaLease I tried doing that, but it doesn't seem to work. I hardcoded the top node path in transform.select but still it wasn't working. I am thinking of adding an extra empty node at the top and the bottom of the editor, will this be the right approach?

What do you mean by hardcording the top node path? I think you should be able to use Editor.start(editor, []) and Editor.end(editor, []) to get the node at the beginning or end, with a path property. Here's an example of where we currently use that: https://github.com/accordproject/web-components/blob/e2e58afdc43f7fc10ca7104a63bebf3c5241a67c/packages/ui-contract-editor/src/components/Clause/index.js#L102

sanketshevkar commented 3 years ago

@DianaLease Since the top node path is [0] so I tried doing Transforms.select(editor, [0]);. Also I wasn't aware of Editor.start(editor, []) & Editor.end(editor, []) as I did not find that in Slate.js's documentation, so thank you. But even that didn't seem to work as it showed the same behavior. Then I used Transforms.moveNodes(editor, { at:, to: }); and it seems to solve the bug. I've committed the changes.

sanketshevkar commented 3 years ago

Here's the video for the same: https://www.loom.com/share/59646c5f29ab4d308d1e4cba850e3daf

jolanglinais commented 3 years ago

Very nice, well done!

sanketshevkar commented 3 years ago

Thanks a lot, @Michael-Grover,@irmerk, and @DianaLease.