Combobox vNext: update useOptionCollection to use DOM ordering instead of React.Children#22572
Conversation
smhigley
force-pushed
the
combobox-dom-ordering
branch
from
ea69d1e to
c4def5d
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e3bf0e8:
|
📊 Bundle size report
🤖 This report was generated against c416a93f71a2a4e9b78fbed720750c275a8cd4ae |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: c416a93f71a2a4e9b78fbed720750c275a8cd4ae (build) |
| } | ||
|
|
||
| // use the DOM method compareDocumentPosition to order the current node against registered nodes | ||
| return Boolean(node.element.compareDocumentPosition(element) === Node.DOCUMENT_POSITION_PRECEDING); |
There was a problem hiding this comment.
I find myself a bit wary of this because we're maintaining internal component state based on the state of rendered DOM which feels backward from the typical React equation of UI = f(state) (where UI is the rendered DOM).
A similar pattern has proven to be a problem Fluent UI v8's List virtualization implementation where DOM measurements are cached in React state but not always updated, leading to scrolling issues.
That said, I don't see any specific issues here, plus this is an internal implementation detail so it could be changed later should it cause problems.
There was a problem hiding this comment.
IMO most of the DOM related stuff on our end tends to be because of our interesting perf stance which is to reduce as many rerenders as possible. React would just say that re-renders are cheap and you should be happy to use them. It's a similar situation with overflow.
However I do agree, that we should always be skeptical when we see native DOM being used in react
layershifter
left a comment
layershifter
left a comment
There was a problem hiding this comment.
It's a huge step forward, great job 🚀
|
This is not related to this PR, but I was also checking what happens with keyboard actions:
We have double rendering for every option even with the latest suggestions. I debugged it and it's happening because - const onOptionClick = (event: React.MouseEvent<HTMLElement>, option: OptionValue) => {
+ const onOptionClick = useEventCallback((event: React.MouseEvent<HTMLElement>, option: OptionValue) => {
// clicked option should always become active option
setActiveOption(getOptionById(option.id));
// close on option click for single-select
!multiselect && setOpen(event, false);
// handle selection change
selectOption(event, option);
- };
+ });With these changes the issue is solved 😉 |
…d of React.Children (microsoft#22572)
…d of React.Children (microsoft#22572)
7 participants
Resolves #22055
I also added a story to show that this works when wrapping
OptionGroupandOption.The most relevant changes are in
useOptionCollection.tsanduseOption.tsx