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

Jump to bottom

[TreeView] Add focusedNode prop #11343

Closed
wants to merge 17 commits into from
Closed

[TreeView] Add focusedNode prop #11343

wants to merge 17 commits into from

Conversation

noraleonte
Copy link
Contributor

@noraleonte noraleonte commented Dec 7, 2023 •
edited
Loading

closes #10236

TODO:

  • add docs

@noraleonte noraleonte added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label Dec 7, 2023
@noraleonte noraleonte self-assigned this Dec 7, 2023
@mui-bot
Copy link

mui-bot commented Dec 7, 2023 •
edited
Loading

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

Updated pages:

Generated by 🚫 dangerJS against 1d8c2d1

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

github-actions bot commented Jan 2, 2024

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

@flaviendelangle flaviendelangle changed the title [TreeView] Add focusedNodeId prop [TreeView] Add focusedNodeId prop Jan 10, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 16, 2024
@flaviendelangle flaviendelangle changed the title [TreeView] Add focusedNodeId prop [TreeView] Add focusedNode prop Jan 17, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2024
@github-actions GitHub Actions
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 Jan 17, 2024
@noraleonte noraleonte marked this pull request as ready for review January 17, 2024 15:47
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 18, 2024
@github-actions GitHub Actions
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 Jan 18, 2024
flaviendelangle
flaviendelangle reviewed Jan 18, 2024
View reviewed changes
packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.types.ts Outdated Show resolved Hide resolved
packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.types.ts Outdated Show resolved Hide resolved
packages/x-tree-view/src/internals/plugins/useTreeViewId/useTreeViewId.types.ts Outdated Show resolved Hide resolved
...ee-view/src/internals/plugins/useTreeViewKeyboardNavigation/useTreeViewKeyboardNavigation.ts Show resolved Hide resolved
packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.ts Show resolved Hide resolved
packages/x-tree-view/src/internals/plugins/useTreeViewFocus/useTreeViewFocus.ts Outdated
@@ -89,10 +79,10 @@ export const useTreeViewFocus: TreeViewPlugin<UseTreeViewFocusSignature> = ({
const createHandleBlur =
(otherHandlers: EventHandlers) => (event: React.FocusEvent<HTMLUListElement>) => {
otherHandlers.onBlur?.(event);
setFocusedNodeId(null);
models.focusedNode.setControlledValue(null);
Copy link
Member

Choose a reason for hiding this comment

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

I would be in favor of having one single function that calls setControlledValue, that way it can also call onFocusedNodeChange without any fear of missing on occurence.

Here, I think it should call onFocusedNodeChange since it's now the callback of a controllable model.
This would require to change the signature of onFocusedNodeChange to accept null

Right now in your doc example, when we blur the Tree View, the previously focused element remains visually focused, which is not the intended behavior IMHO.

But this raises another question: if the Tree View is not focused and focusedNode != null, should we display the item as focused? I don't think so, I think that isNodeFocused should check if the Tree View is focused.
I would like to have the opinion of others on this one @LukasTy @alexfauquette

Copy link
Member

Choose a reason for hiding this comment

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

I may lack of context, but from my past experience on the data grid and pickers, each time we tracked a focused item, if the component don't have focus, it's not visible, but used to initial the focus when it comes back.

Here is a data grid example of the focus coming back to the last focused cell

Screencast.from.22-01-2024.14.49.00.mp4

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 19, 2024
@github-actions GitHub Actions
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 Jan 22, 2024
@github-actions GitHub Actions
Copy link

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

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

I opened a separate PR for this to keep things clean. Closing this one in favor of #12143

@noraleonte noraleonte closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexfauquette alexfauquette alexfauquette left review comments

@flaviendelangle flaviendelangle flaviendelangle left review comments

Assignees

@noraleonte noraleonte

Labels
component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TreeView] Add prop to TreeView component to control focused node
4 participants
@noraleonte @mui-bot @flaviendelangle @alexfauquette