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.

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] Remove instance.getTreeItemIdAttribute #14667

Merged
merged 9 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion packages/x-tree-view/src/TreeItem/TreeItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { TreeViewItemDepthContext } from '../internals/TreeViewItemDepthContext'
import { useTreeItemState } from './useTreeItemState';
import { isTargetInDescendants } from '../internals/utils/tree';
import { TreeViewItemPluginSlotPropsEnhancerParams } from '../internals/models';
import { generateTreeItemIdAttribute } from '../internals/corePlugins/useTreeViewId/useTreeViewId.utils';

const useThemeProps = createUseThemeProps('MuiTreeItem');

Expand Down Expand Up @@ -199,6 +200,7 @@ export const TreeItem = React.forwardRef(function TreeItem(
items: { disabledItemsFocusable, indentationAtItemLevel },
selection: { multiSelect },
expansion: { expansionTrigger },
treeId,
instance,
} = useTreeViewContext<TreeItemMinimalPlugins, TreeItemOptionalPlugins>();
const depthContext = React.useContext(TreeViewItemDepthContext);
Expand Down Expand Up @@ -382,7 +384,7 @@ export const TreeItem = React.forwardRef(function TreeItem(
instance.handleItemKeyDown(event, itemId);
};

const idAttribute = instance.getTreeItemIdAttribute(itemId, id);
const idAttribute = generateTreeItemIdAttribute({ itemId, treeId, id });
const tabIndex = instance.canItemBeTabbed(itemId) ? 0 : -1;

const sharedPropsEnhancerParams: Omit<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { useTreeViewContext } from './useTreeViewContext';
import { escapeOperandAttributeSelector } from '../utils/utils';
import type { UseTreeViewJSXItemsSignature } from '../plugins/useTreeViewJSXItems';
import type { UseTreeViewItemsSignature } from '../plugins/useTreeViewItems';
import { generateTreeItemIdAttribute } from '../corePlugins/useTreeViewId/useTreeViewId.utils';

export const TreeViewChildrenItemContext =
React.createContext<TreeViewChildrenItemContextValue | null>(null);
Expand All @@ -20,7 +21,7 @@ interface TreeViewChildrenItemProviderProps {
export function TreeViewChildrenItemProvider(props: TreeViewChildrenItemProviderProps) {
const { children, itemId = null } = props;

const { instance, rootRef } =
const { instance, treeId, rootRef } =
useTreeViewContext<[UseTreeViewJSXItemsSignature, UseTreeViewItemsSignature]>();
const childrenIdAttrToIdRef = React.useRef<Map<string, string>>(new Map());

Expand All @@ -36,7 +37,7 @@ export function TreeViewChildrenItemProvider(props: TreeViewChildrenItemProvider
// Undefined during 1st render
const itemMeta = instance.getItemMeta(itemId);
if (itemMeta !== undefined) {
idAttr = instance.getTreeItemIdAttribute(itemId, itemMeta.idAttribute);
idAttr = generateTreeItemIdAttribute({ itemId, treeId, id: itemMeta.idAttribute });
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export function TreeViewProvider<TSignatures extends readonly TreeViewAnyPluginS

return (
<TreeViewContext.Provider value={value}>
{value.wrapRoot({ children })}
{value.wrapRoot({ children, instance: value.instance })}
</TreeViewContext.Provider>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
TreeViewItemPluginResponse,
TreeViewPublicAPI,
} from '../models';
import { TreeViewCorePluginSignatures } from '../corePlugins';

export type TreeViewItemPluginsRunner = <TProps extends {}>(
props: TProps,
Expand All @@ -16,7 +17,7 @@ export type TreeViewItemPluginsRunner = <TProps extends {}>(
export type TreeViewContextValue<
TSignatures extends readonly TreeViewAnyPluginSignature[],
TOptionalSignatures extends readonly TreeViewAnyPluginSignature[] = [],
> = MergeSignaturesProperty<TSignatures, 'contextValue'> &
> = MergeSignaturesProperty<[...TreeViewCorePluginSignatures, ...TSignatures], 'contextValue'> &
Partial<MergeSignaturesProperty<TOptionalSignatures, 'contextValue'>> & {
instance: TreeViewInstance<TSignatures, TOptionalSignatures>;
publicAPI: TreeViewPublicAPI<TSignatures, TOptionalSignatures>;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,26 +1,40 @@
import * as React from 'react';
import useId from '@mui/utils/useId';
import { TreeViewPlugin } from '../../models';
import { UseTreeViewIdSignature } from './useTreeViewId.types';
import { createTreeViewDefaultId } from './useTreeViewId.utils';

export const useTreeViewId: TreeViewPlugin<UseTreeViewIdSignature> = ({ params }) => {
const treeId = useId(params.id);
export const useTreeViewId: TreeViewPlugin<UseTreeViewIdSignature> = ({
params,
state,
setState,
}) => {
const treeId = state.id.treeId;

const getTreeItemIdAttribute = React.useCallback(
(itemId: string, idAttribute: string | undefined) => idAttribute ?? `${treeId}-${itemId}`,
[treeId],
);
React.useEffect(() => {
setState((prevState) => {
if (prevState.id.treeId === params.id) {
return prevState;
}

return {
...prevState,
id: { ...prevState.id, treeId: params.id ?? createTreeViewDefaultId() },
};
});
}, [setState, params.id]);

return {
getRootProps: () => ({
id: treeId,
}),
instance: {
getTreeItemIdAttribute,
contextValue: {
treeId,
},
};
};

useTreeViewId.params = {
id: true,
};

useTreeViewId.getInitialState = ({ id }) => ({ id: { treeId: id ?? createTreeViewDefaultId() } });
Original file line number Diff line number Diff line change
@@ -1,17 +1,4 @@
import { TreeViewPluginSignature } from '../../models';
import { TreeViewItemId } from '../../../models';

export interface UseTreeViewIdInstance {
/**
* Get the id attribute (i.e.: the `id` attribute passed to the DOM element) of a tree item.
* If the user explicitly defined an id attribute, it will be returned.
* Otherwise, the method created a unique id for the item based on the Tree View id attribute and the item `itemId`
* @param {TreeViewItemId} itemId The id of the item to get the id attribute of.
* @param {string | undefined} idAttribute The id attribute of the item if explicitly defined by the user.
* @returns {string} The id attribute of the item.
*/
getTreeItemIdAttribute: (itemId: TreeViewItemId, idAttribute: string | undefined) => string;
}

export interface UseTreeViewIdParameters {
/**
Expand All @@ -23,8 +10,19 @@ export interface UseTreeViewIdParameters {

export type UseTreeViewIdDefaultizedParameters = UseTreeViewIdParameters;

export interface UseTreeViewIdState {
id: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question: any reason why we are nesting treeId? Is it to be able to identify the state related to a certain plugin more easily? Should we do the same for other plugin states?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it to be able to identify the state related to a certain plugin more easily?

Yes, I took the habit to namespace everything in the state

Should we do the same for other plugin states?

Do we have some plugins where it is not the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

UseTreeViewFocusState and UseTreeViewLabelState do not follow this convention from what I see

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I fixed those in #14210 👼
But that's something I should definitely extract

treeId: string;
};
}

interface UseTreeViewIdContextValue {
treeId: string | undefined;
}

export type UseTreeViewIdSignature = TreeViewPluginSignature<{
params: UseTreeViewIdParameters;
defaultizedParams: UseTreeViewIdDefaultizedParameters;
instance: UseTreeViewIdInstance;
state: UseTreeViewIdState;
contextValue: UseTreeViewIdContextValue;
}>;
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { TreeViewItemId } from '../../../models';

let globalTreeViewDefaultId = 0;
export const createTreeViewDefaultId = () => {
globalTreeViewDefaultId += 1;
return `mui-tree-view-${globalTreeViewDefaultId}`;
};

/**
* Generate the id attribute (i.e.: the `id` attribute passed to the DOM element) of a tree item.
* If the user explicitly defined an id attribute, it will be returned.
* Otherwise, the method creates a unique id for the item based on the Tree View id attribute and the item `itemId`
* @param {object} params The parameters to determine the id attribute of the item.
* @param {TreeViewItemId} params.itemId The id of the item to get the id attribute of.
* @param {string | undefined} params.idAttribute The id attribute of the item if explicitly defined by the user.
* @param {string} params.treeId The id attribute of the Tree View.
* @returns {string} The id attribute of the item.
*/
export const generateTreeItemIdAttribute = ({
id,
treeId = '',
itemId,
}: {
id: TreeViewItemId | undefined;
treeId: string | undefined;
itemId: string;
}): string => {
if (id != null) {
return id;
}

return `${treeId}-${itemId}`;
};
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,7 @@ export const useTreeViewFocus: TreeViewPlugin<UseTreeViewFocusSignature> = ({

const itemMeta = instance.getItemMeta(state.focusedItemId);
if (itemMeta) {
const itemElement = document.getElementById(
instance.getTreeItemIdAttribute(state.focusedItemId, itemMeta.idAttribute),
);

const itemElement = instance.getItemDOMElement(state.focusedItemId);
if (itemElement) {
itemElement.blur();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { publishTreeViewEvent } from '../../utils/publishTreeViewEvent';
import { TreeViewBaseItem, TreeViewItemId } from '../../../models';
import { buildSiblingIndexes, TREE_VIEW_ROOT_PARENT_ID } from './useTreeViewItems.utils';
import { TreeViewItemDepthContext } from '../../TreeViewItemDepthContext';
import { generateTreeItemIdAttribute } from '../../corePlugins/useTreeViewId/useTreeViewId.utils';

interface UpdateNodesStateParameters
extends Pick<
Expand Down Expand Up @@ -180,7 +181,9 @@ export const useTreeViewItems: TreeViewPlugin<UseTreeViewItemsSignature> = ({
return null;
}

return document.getElementById(instance.getTreeItemIdAttribute(itemId, itemMeta.idAttribute));
return document.getElementById(
generateTreeItemIdAttribute({ treeId: state.id.treeId, itemId, id: itemMeta.idAttribute }),
);
};

const isItemNavigable = (itemId: string) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
import type { TreeItemProps } from '../../../TreeItem';
import type { TreeItem2Props } from '../../../TreeItem2';
import { TreeViewItemDepthContext } from '../../TreeViewItemDepthContext';
import { generateTreeItemIdAttribute } from '../../corePlugins/useTreeViewId/useTreeViewId.utils';

export const useTreeViewJSXItems: TreeViewPlugin<UseTreeViewJSXItemsSignature> = ({
instance,
Expand Down Expand Up @@ -121,7 +122,7 @@ const useTreeViewJSXItemsItemPlugin: TreeViewItemPlugin<TreeItemProps | TreeItem
rootRef,
contentRef,
}) => {
const { instance } = useTreeViewContext<[UseTreeViewJSXItemsSignature]>();
const { instance, treeId } = useTreeViewContext<[UseTreeViewJSXItemsSignature]>();
const { children, disabled = false, label, itemId, id } = props;

const parentContext = React.useContext(TreeViewChildrenItemContext);
Expand All @@ -142,13 +143,13 @@ const useTreeViewJSXItemsItemPlugin: TreeViewItemPlugin<TreeItemProps | TreeItem

// Prevent any flashing
useEnhancedEffect(() => {
const idAttributeWithDefault = instance.getTreeItemIdAttribute(itemId, id);
registerChild(idAttributeWithDefault, itemId);
const idAttribute = generateTreeItemIdAttribute({ itemId, treeId, id });
registerChild(idAttribute, itemId);

return () => {
unregisterChild(idAttributeWithDefault);
unregisterChild(idAttribute);
};
}, [instance, registerChild, unregisterChild, itemId, id]);
}, [registerChild, unregisterChild, itemId, id, treeId]);

React.useEffect(() => {
return instance.insertJSXItem({
Expand Down
5 changes: 4 additions & 1 deletion packages/x-tree-view/src/useTreeItem2/useTreeItem2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
import { useTreeItem2Utils } from '../hooks/useTreeItem2Utils';
import { TreeViewItemDepthContext } from '../internals/TreeViewItemDepthContext';
import { isTargetInDescendants } from '../internals/utils/tree';
import { generateTreeItemIdAttribute } from '../internals/corePlugins/useTreeViewId/useTreeViewId.utils';

export const useTreeItem2 = <
TSignatures extends UseTreeItem2MinimalPlugins = UseTreeItem2MinimalPlugins,
Expand All @@ -38,6 +39,7 @@ export const useTreeItem2 = <
items: { onItemClick, disabledItemsFocusable, indentationAtItemLevel },
selection: { multiSelect, disableSelection, checkboxSelection },
expansion: { expansionTrigger },
treeId,
instance,
publicAPI,
} = useTreeViewContext<TSignatures, TOptionalSignatures>();
Expand All @@ -49,10 +51,11 @@ export const useTreeItem2 = <
const { interactions, status } = useTreeItem2Utils({ itemId, children });
const rootRefObject = React.useRef<HTMLLIElement>(null);
const contentRefObject = React.useRef<HTMLDivElement>(null);
const idAttribute = instance.getTreeItemIdAttribute(itemId, id);
const handleRootRef = useForkRef(rootRef, pluginRootRef, rootRefObject)!;
const handleContentRef = useForkRef(contentRef, contentRefObject)!;
const checkboxRef = React.useRef<HTMLButtonElement>(null);

const idAttribute = generateTreeItemIdAttribute({ itemId, treeId, id });
const rootTabIndex = instance.canItemBeTabbed(itemId) ? 0 : -1;

const sharedPropsEnhancerParams: Omit<
Expand Down
2 changes: 1 addition & 1 deletion test/utils/tree-view/fakeContextValue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export const getFakeContextValue = (
isItemFocused: () => false,
isItemSelected: () => false,
isItemDisabled: (itemId: string | null): itemId is string => !!itemId,
getTreeItemIdAttribute: () => '',
mapFirstCharFromJSX: () => () => {},
canItemBeTabbed: () => false,
} as any,
Expand Down Expand Up @@ -43,6 +42,7 @@ export const getFakeContextValue = (
checkboxSelection: features.checkboxSelection ?? false,
disableSelection: false,
},
treeId: 'mui-tree-view-1',
rootRef: {
current: null,
},
Expand Down