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] Allow to define indentation at the item level #13126

Merged
merged 17 commits into from
May 23, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented May 14, 2024

Part of #9686

Usage

<RichTreeView items={MUI_X_PRODUCTS} experimentalFeatures={{ indentationAtItemLevel: true }} />

(same on SimpleTreeView and RichTreeViewPro)

Problem

When using the drag & drop, we need to have an element that goes from the far left of the Tree View (at the left border of an element of depth 0) to the far right.
But right now this is not easily doable because of how the padding are handled on the Tree View (it's the groupTransition slot which sets the padding to visually indent its children).

Solution

One solution would be to switch to a strategy where each it content sets its padding-left depending on its depth.
This PR proposes one potential implementation, opt-in via an experimentalFeature flag (similar to what the grid is doing for breaking changes during minor).
This flag would be required in order for drag and drop to work (we can add a runtime warning in dev if not enabled, a warning on the JSDoc on the itemsReordering prop and a warning on the doc).
In v8 we would drop this flag.

Limitation

Vertical border

It becomes harder to do vertical borders like on this example:

image

For me the best solution here would be to not use the flag for now, and in v8 to override the padding on the content slot and add a padding on the groupTransition slot (going back to the v7 default behavior).
This would not be compatible with the drag & drop but I don't think this UI is meant to be used with drag & drop anyway.

Fragile with padding customization

The fact that the drag & drop relies on the position of the content slot can make it quite fragile, whatever padding position we have.
We can lower the risk by creating a prop indentPerLevel (implemented in #12213) that allows people to change the indentation offset per level without touching the CSS and without doing things that would not be compatible with the drag&drop.

@flaviendelangle flaviendelangle self-assigned this May 14, 2024
@flaviendelangle flaviendelangle added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label May 14, 2024
@mui-bot
Copy link

mui-bot commented May 14, 2024

Deploy preview: https://deploy-preview-13126--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 984f50d

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 16, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 16, 2024
@@ -194,6 +194,14 @@ RichTreeView.propTypes = {
* Used when the item's expansion is controlled.
*/
expandedItems: PropTypes.arrayOf(PropTypes.string),
/**
Copy link
Member Author

Choose a reason for hiding this comment

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

Description copy pasted from the grid

import { TreeViewItemId } from '../../models';

export const TreeViewItemDepthContext = React.createContext<
number | ((itemId: TreeViewItemId) => number)
Copy link
Member Author

Choose a reason for hiding this comment

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

SimpleTreeView uses a number because the depth of an item is always the depth of its parent + 1
RichTreeView uses a function to retrieve the depth from the state

Not sure how to improve this...

.filter((wrapRoot): wrapRoot is TreeRootWrapper => !!wrapRoot);
.filter((wrapRoot): wrapRoot is TreeRootWrapper<any> => !!wrapRoot)
// The wrappers are reversed to ensure that the first wrapper is the outermost one.
.reverse();
Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise the provider of useTreeViewItems takes precedence over useTreeVIewJSXItems which messes the behavior on SimpleTreeView

IMHO it make sense that if two plugins wrap the root (or the items), then it's the last executed plugin that has its wrappers deeper in the component tree and thus executed last.

@@ -148,21 +151,36 @@ export const useTreeItem2 = <TPlugins extends DefaultTreeViewPlugins = DefaultTr
onBlur: createRootHandleBlur(externalEventHandlers),
onKeyDown: createRootHandleKeyDown(externalEventHandlers),
};

if (indentationAtItemLevel) {
Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid any breaking change, I'm only adding those props when the experimental feature is enabled.
When not using the experimental feature, the props passed to each slot remain unchanged.

@flaviendelangle flaviendelangle changed the title [TreeView] Allow to define indentation at the item level [WIP] [TreeView] Allow to define indentation at the item level May 20, 2024
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

The solution seems to make sense. 👍
I'm not sure if we can make it better without a BC.
A couple of things to note:

  1. Do we want the items to always have the style defined regardless of the flag state?
style="--TreeView-itemDepth: 2;"
  1. It does not seem to work (be reflected) on the headless API.

@flaviendelangle
Copy link
Member Author

Do we want the items to always have the style defined regardless of the flag state?

I only added it when the flag is enabled for TreeItem2 but forgot the check on TreeItem, I'll fix it

@flaviendelangle
Copy link
Member Author

It does not seem to work (be reflected) on the headless API

This demo sets a custom padding on TreeItem2Content.
It's not using the flag so everything should work fine, but if you enable it, you end up with a flag tree since the padding we put is overridden.

Not sure how to handle that one...
Either we say that this demo is not using the feature so it's fine (maybe adding a small note below the demo to make it clear)
Or we enable the flag on this demo and adapt the padding accordingly.
Or we add a new div inside CustomTreeItemContent just to add the padding on another element than the one used internally for indentation when the flag is enabled.

This is super specific to our doc though, in a real application, people are either using the flag or not using it, they will never try to create a Tree View that supports both.

@flaviendelangle flaviendelangle requested a review from LukasTy May 23, 2024 12:22
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM with the following addressed:

I only added it when the flag is enabled for TreeItem2 but forgot the check on TreeItem, I'll fix it

P.S. It would be nice to have this feature/prop documented somehwere, but I take it that you plan to do it together with the drag&drop feature? 🤔
However, that feature is only for the pro package, whereas this prop impacts every TreeView component. 🤷

@flaviendelangle
Copy link
Member Author

P.S. It would be nice to have this feature/prop documented somehwere, but I take it that you plan to do it together with the drag&drop feature?

I was planning on doing a follow up with the indentPerLevel prop extracted from the drag&drop PR and some doc for both the experimental feature and the new prop.
Without the new prop, the experimental feature is limited when you try to customize.

@flaviendelangle flaviendelangle merged commit b6b7e89 into mui:master May 23, 2024
15 checks passed
@flaviendelangle flaviendelangle deleted the indendation-item branch May 23, 2024 14:54
arthurbalduini pushed a commit to arthurbalduini/mui-x that referenced this pull request May 23, 2024
DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants