[TreeView] Rework the selection internals #12703
Conversation
flaviendelangle
added
performance
component: tree view
|
Deploy preview: https://deploy-preview-12703--material-ui-x.netlify.app/ |
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
PR: out-of-date
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
PR: out-of-date
flaviendelangle
force-pushed
the
selection-refacto
branch
from
dca3ac7
to
87388a5
Compare
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
PR: out-of-date
| itemId: string, | ||
| keepExistingSelection?: boolean, | ||
| ) => void; | ||
| /** |
There was a problem hiding this comment.
I split the API into several methods instead of a single selectRange
That way each one can receive the parameters it needs AND we can apply some optimization (selectAllNavigableItems is a lot cheaper now, no need to try to reconcile the old and the new model, you just select everything).
There was a problem hiding this comment.
It makes a lot more sense like this! 👌 The previous API felt a little bit like mental gymnastics to me, this feels a lot more coherent 💯
| * @param {string} currentItemId The id of the active item before the keyboard navigation. | ||
| * @param {string} nextItemId The id of the active item after the keyboard navigation. | ||
| */ | ||
| selectItemFromArrowNavigation: ( |
There was a problem hiding this comment.
Not a huge fan of this abstraction, I would prefer useTreeViewKeyboardNavigation to only use methods that are not explicitly related to keyboard navigation.
But the selection behavior of the arrows require stuff that is inside useTreeViewSelection and I don't see how to name this method differently 😬
There was a problem hiding this comment.
But the selection behavior of the arrows require stuff that is inside useTreeViewSelection
makes sense to me. I think the naming is fine here tbh 🤔
There was a problem hiding this comment.
Amazing work 👌 🎉
The changes make a lot of sense to me, and the API is much better now IMO 💯
Regarding the behavior change when pressing Home then Enter, I think the approach you went with is the most logical. I would be ok with 1 too, but personally prefer 2
| itemId: string, | ||
| keepExistingSelection?: boolean, | ||
| ) => void; | ||
| /** |
There was a problem hiding this comment.
It makes a lot more sense like this! 👌 The previous API felt a little bit like mental gymnastics to me, this feels a lot more coherent 💯
| * @param {string} currentItemId The id of the active item before the keyboard navigation. | ||
| * @param {string} nextItemId The id of the active item after the keyboard navigation. | ||
| */ | ||
| selectItemFromArrowNavigation: ( |
There was a problem hiding this comment.
But the selection behavior of the arrows require stuff that is inside useTreeViewSelection
makes sense to me. I think the naming is fine here tbh 🤔
Part of #10300
Goal
TreeViewItemRangeinterface (understanding what was going on was a pain with this object of 4 optional properties)Behavior change
This PR includes a very small behavior change when using the Home or End key.
Before, when pressing Home, the focus was moving to the first navigable item and when pressing End the focus was moving to the last navigable item.
Additionally, if you pressed Home then End, the end selection was all the items below the item originally focus.
For me this is inconsistent, either:
Screencast.2024-04-11.15.36.29.mp4
Screencast.2024-04-11.15.34.57.mp4
Here is what the ARIA spec says:
I went for 2., but both are clearly totally valid