Skip to content
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.

Sign up for GitHub

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

[TreeView] Fix drag and drop issue with label editing #15636

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+40 −14
Open

[TreeView] Fix drag and drop issue with label editing #15636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6b37ea3
fix d&d issue + label editing click
noraleonte Nov 27, 2024
4c200fb
review - Flavien
noraleonte Dec 4, 2024
ff7eab0
Merge branch 'master' of github.com:noraleonte/mui-x into fix-reorder…
noraleonte Dec 4, 2024
6e26f53
disable dnd when editing
noraleonte Dec 16, 2024
dd339ee
Merge branch 'master' of github.com:noraleonte/mui-x into fix-reorder…
noraleonte Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions ...src/internals/plugins/useTreeViewItemsReordering/useTreeViewItemsReordering.itemPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import {
UseTreeViewItemsSignature,
isTargetInDescendants,
useSelector,
selectorIsAnItemEdited,
UseTreeViewLabelSignature,
} from '@mui/x-tree-view/internals';
import {
UseTreeItemDragAndDropOverlaySlotPropsFromItemsReordering,
Expand All @@ -23,7 +25,9 @@ export const isAndroid = () => navigator.userAgent.toLowerCase().includes('andro

export const useTreeViewItemsReorderingItemPlugin: TreeViewItemPlugin = ({ props }) => {
const { instance, store, itemsReordering } =
useTreeViewContext<[UseTreeViewItemsSignature, UseTreeViewItemsReorderingSignature]>();
useTreeViewContext<
[UseTreeViewItemsSignature, UseTreeViewItemsReorderingSignature, UseTreeViewLabelSignature]
>();
const { itemId } = props;

const validActionsRef = React.useRef<TreeViewItemItemReorderingValidActions | null>(null);
Expand All @@ -35,6 +39,8 @@ export const useTreeViewItemsReorderingItemPlugin: TreeViewItemPlugin = ({ props
);
const isValidTarget = useSelector(store, selectorItemsReorderingIsValidTarget, itemId);

const isEditing = useSelector(store, selectorIsAnItemEdited);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will re-render all the items when starting editing.
In term of perfs, I think the best trade-off would be:

  1. Use !!isItemEditable instead of isEditing on line 54 (if the label editing is not configured at all then we don't create the callbacks

  2. Use the selector to know if an item is being edited inside handleDragStart, handleRootDragOver and handleRootDragEnd

The only diff is that draggable would be true even when editing an item, not sure if thats a problem.
If it is, then I don't have a great solution to avoid the full re-render 😬


return {
propsEnhancers: {
root: ({
Expand All @@ -44,7 +50,8 @@ export const useTreeViewItemsReorderingItemPlugin: TreeViewItemPlugin = ({ props
}): UseTreeItemRootSlotPropsFromItemsReordering => {
if (
!itemsReordering.enabled ||
(itemsReordering.isItemReorderable && !itemsReordering.isItemReorderable(itemId))
(itemsReordering.isItemReorderable && !itemsReordering.isItemReorderable(itemId)) ||
isEditing
) {
return {};
}
Expand Down Expand Up @@ -79,6 +86,7 @@ export const useTreeViewItemsReorderingItemPlugin: TreeViewItemPlugin = ({ props

const handleRootDragOver = (event: React.DragEvent & TreeViewCancellableEvent) => {
externalEventHandlers.onDragOver?.(event);

if (event.defaultMuiPrevented) {
return;
}
Expand All @@ -88,6 +96,7 @@ export const useTreeViewItemsReorderingItemPlugin: TreeViewItemPlugin = ({ props

const handleRootDragEnd = (event: React.DragEvent & TreeViewCancellableEvent) => {
externalEventHandlers.onDragEnd?.(event);

if (event.defaultMuiPrevented) {
return;
}
Expand All @@ -112,6 +121,7 @@ export const useTreeViewItemsReorderingItemPlugin: TreeViewItemPlugin = ({ props

const handleDragOver = (event: React.DragEvent & TreeViewCancellableEvent) => {
externalEventHandlers.onDragOver?.(event);

if (event.defaultMuiPrevented || validActionsRef.current == null) {
return;
}
Expand Down
22 changes: 13 additions & 9 deletions ...e-view-pro/src/internals/plugins/useTreeViewItemsReordering/useTreeViewItemsReordering.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react';
import {
TreeViewPlugin,
selectorIsItemBeingEdited,
selectorItemIndex,
selectorItemMeta,
selectorItemOrderedChildrenIds,
Expand Down Expand Up @@ -131,15 +132,18 @@ export const useTreeViewItemsReordering: TreeViewPlugin<UseTreeViewItemsReorderi

const startDraggingItem = React.useCallback(
(itemId: string) => {
store.update((prevState) => ({
...prevState,
itemsReordering: {
targetItemId: itemId,
draggedItemId: itemId,
action: null,
newPosition: null,
},
}));
const itemBeingEdited = selectorIsItemBeingEdited(store.value, itemId);
if (!itemBeingEdited) {
store.update((prevState) => ({
...prevState,
itemsReordering: {
targetItemId: itemId,
draggedItemId: itemId,
action: null,
newPosition: null,
},
}));
}
},
[store],
);
Expand Down
2 changes: 1 addition & 1 deletion packages/x-tree-view/src/hooks/useTreeItemUtils/useTreeItemUtils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export const useTreeItemUtils = <
return;
}

if (!status.focused) {
if (!status.focused && !status.editing) {
instance.focusItem(event, itemId);
}

Expand Down
4 changes: 4 additions & 0 deletions packages/x-tree-view/src/internals/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ export type {
UseTreeViewItemsState,
} from './plugins/useTreeViewItems';
export { useTreeViewLabel } from './plugins/useTreeViewLabel';
export {
selectorIsItemBeingEdited,
selectorIsAnItemEdited,
} from './plugins/useTreeViewLabel/useTreeViewLabel.selectors';
export type {
UseTreeViewLabelSignature,
UseTreeViewLabelParameters,
Expand Down
12 changes: 10 additions & 2 deletions packages/x-tree-view/src/internals/plugins/useTreeViewLabel/useTreeViewLabel.selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ export const selectorIsItemEditable = createSelector(
* @returns {boolean} `true` if the item is being edited, `false` otherwise.
*/
export const selectorIsItemBeingEdited = createSelector(
[selectorTreeViewLabelState, (_, itemId: string) => itemId],
(labelState, itemId) => labelState.editedItemId === itemId,
[selectorTreeViewLabelState, (_, itemId: string | null) => itemId],
(labelState, itemId) => (itemId ? labelState.editedItemId === itemId : false),
);
/**
* Check if an item is being edited.
* @param {TreeViewState<[UseTreeViewLabelSignature]>} state The state of the tree view.
* @returns {boolean} `true` if there is an item that is being edited, `false` otherwise.
*/
export const selectorIsAnItemEdited = createSelector(selectorTreeViewLabelState, (labelState) =>
Boolean(labelState?.editedItemId),
);
Loading