-
Notifications
You must be signed in to change notification settings - Fork 504
feat(@clayui/core): implements DnD in TreeView #4307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
matuzalemsteles
merged 15 commits into
liferay:master
from
matuzalemsteles:issue/4207.1
Sep 30, 2021
Merged
feat(@clayui/core): implements DnD in TreeView #4307
matuzalemsteles
merged 15 commits into
liferay:master
from
matuzalemsteles:issue/4207.1
Sep 30, 2021
Conversation
This file contains hidden or 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
…e item in drop action This is just a draft for an attempt to implement RFC 6902 (JSON Patch), it also won't cover all the cases that the RFC covers but uses the standards to implement the `move` operation which is what interests us most for perform the `drop` operation, also somewhat based on the Immer API which is used for immutability. It still has to add the implementation of applying patches and dealing with immutability which is being very difficult to do. In section 4.4 the `path` and `from` are a string but we are using an array of indices to simplify the case here. I'm not considering adding the index to tree structure to `items` because that would be an operation we need to call again when implementing "load more" and it's a little expensive depending on the depth and size, just testing other alternatives.
chore(@clayui/core): clean up move operation in `useTree` chore(@clayui/core): add missing types chore(@clayui/core): reorder child nodes in the same root node chore: regen yarn.lock chore(@clayui/core): make move operation work with nested children
chore(@clayui/core): fixes error when not including notice header to DragLayer
…external value and internal controller This allowed to cause some problems when the `value` has some value but doesn't have an `onChange`, it caused the `onChange` to be used the internal one but the `value` is the external one, we should always keep the uniform behavior, if the `onChange` or `value` doesn't have a value, it must use the internal state.
`open` is called several times when the item is moving, it causes a lot of slowness when moving the item, and frame drop because although we don't add anything to the `expandedKeys` we create a copy of it and we will save it, it schedules a new one task in the React Scheduler, as we do this over and over again, we are flooding with tasks. fix(@clayui/core): fixes error when getting items from props instead of state fix(@clayui/core): fix error when starting internal state As we fixed the behavior of `useInternalState` to follow the behavior of if it doesn't have any of the properties it will use the internal state, we also need to support the initial state, if the initial state is not passed but there is a value in `value` prop, we should use it as state initial.
…copying the partial structure The strategy here is to make a partial copy of the structure to deal with the immutable tree but reusing the paths that are not modified, only the paths that are modified undergo changes in the tree that automatically create pointers to the original tree. The cost is significantly less compared to creating a copy of the tree and then modifying it because we would need to traverse the entire tree to create a copy. We also avoid creating a partial copy before modifying, we create the partial copy in the path iterable. fix(@clayui/chore): fix error when not passing `nestedKey` to context
chore(@clayui/core): add missing `nestedKey` prop chore(@clayui/core): add test for nested items chore(@clayui/core): pass `overTarget` prop to `TreeViewItem` chore(@clayui/core): drop either in a parent node or child node chore(@clayui/core): fix tsc errors
…view-dropping-inside` and `treeview-dropping-top` chore(@clayui/core): replace `treeview-dropping-inside` class by `treeview-dropping-middle`
chore(@clayui/core): fix TypeScript error
…ldren chore(@clayui/core): adds more test cases by moving nested items This covers most use cases when moving an item, moving a nested item, persisting the items, moving to the last item in the list. fix(@clayui/core): fixes error when moving the item inside the item and its children chore(@clayui/core): fixes the test of the operation the move item on the object The behavior is now different, if the indexes are the same size, it means you must move the item to the same level of the hierarchy but if you want to move the item inside an item add a new index 0 to path. chore(@clayui/core): add the test case by moving the item above and below an item This case is very simple and doesn't cover all the use cases of moving the item up and down over an item, add in the next interactions: - Move item above or below a nested item - Move the item below the last item in the hierarchy - Move the item above the first item in the hierarchy fix(@clayui/core): fixes error when adding an empty item in the structure when navigating indexes This fixes two main issues when using indexes to move items: - Array can change the position of items when deleting first the item - Move an item below the last item chore(@clayui/core): open the item only when hovering over the element for a few ms When moving an item that has a lot of nesting, opening as it moves the item to the desired place causes a bad experience, especially if the items have many items, use the strategy of opening only if it stays still on the item for a few ms that shows the intention to open the item instead of opening just swiping when you don't have an intention to open. chore: regen yarn.lock Some things got lost during the PR rebase with the master.
julien
approved these changes
Sep 28, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Well I'm going ahead with that and merging but feel free to look at the PR, comment and if you need anything else we can send you anytime. @bryceosterhaus feel free to look at the implementation as well. |
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.
This is the final implementation for the TreeView implementation of DnD, this PR contains some commits that were squash to reduce the noise a bit, I like to keep it because normally the commit gets too big and confusing to understand but we can talk about that later. The entire context of implementing this functionality is in PR #4255.
I'm also sending another PR to keep the other PR's history available.