fix(combobox): fix virtualization ref bug #1922
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: b83ef38 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
Area: Components
andioneto
commented
Oct 6, 2021
| paddingStart: remToPx(theme.space.space30, 'number') as number, | ||
| }); | ||
|
|
||
| const defaultState = useComboboxPrimitive({ |
There was a problem hiding this comment.
ℹ️ Removed the conditional use of the virtualizer and the primitive hooks because according to the rules of hooks, hooks should not be called conditionally.
andioneto
commented
Oct 6, 2021
| ...(inputValue != null && {inputValue}), | ||
| ...(selectedItem != null && {selectedItem}), | ||
| }); | ||
| } = state || defaultState; |
There was a problem hiding this comment.
Conditionally destructure the targets.
andioneto
commented
Oct 6, 2021
| getItemProps={getItemProps} | ||
| highlightedIndex={highlightedIndex} | ||
| optionTemplate={optionTemplate} | ||
| groupItemsBy={groupItemsBy} | ||
| groupLabelTemplate={groupLabelTemplate} | ||
| totalSize={rowVirtualizer.totalSize} |
There was a problem hiding this comment.
Flattening the props to the extent that we can, general good practice for performance in React.
andioneto
commented
Oct 6, 2021
|
|
||
| // Use option template if provided | ||
| // otherwise, return the items array. | ||
| const templatizedItems = React.useMemo(() => { |
There was a problem hiding this comment.
Create a memoized list of the items that uses the option template if it exists, and doesn't use it if it doesn't exist.
andioneto
commented
Oct 6, 2021
| }) => { | ||
| const UIDSeed = useUIDSeed(); | ||
| const uidSeed = useUIDSeed(); |
|
✔️ Deploy Preview for paste-theme-designer ready! 🔨 Explore the source changes: b83ef38 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-theme-designer/deploys/61670c98462c1400081c2833 😎 Browse the preview: https://deploy-preview-1922--paste-theme-designer.netlify.app |
andioneto
commented
Oct 6, 2021
| variant="default" | ||
| virtualItem={virtualItem} | ||
| aria-setsize={items.length} | ||
| aria-posinset={items.indexOf(item)} | ||
| aria-setsize={totalSize} |
There was a problem hiding this comment.
For resizing accuracy, we should use the list size from the virtualized list.
andioneto
commented
Oct 6, 2021
| // @ts-ignore | ||
| ref={(currentElement) => virtualItem.measureRef(currentElement)} | ||
| {...getItemProps({item, index: itemIndex})} | ||
| {...getItemProps({item, index: itemIndex, ref: virtualItem.measureRef})} |
There was a problem hiding this comment.
getItemProps allows for a ref to be passed in this invocation. This allows us to "combine" the refs from the virtualization and the combobox primitive. Check out the Downshift.js docs for more context.
|
✔️ Deploy Preview for paste-docs ready! 🔨 Explore the source changes: aac7932 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-docs/deploys/615e3507dd0c7c0007d7e638 😎 Browse the preview: https://deploy-preview-1922--paste-docs.netlify.app |
|
Size Change: +37 B (0%) Total Size: 654 kB
ℹ️ View Unchanged
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
|
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 b83ef38:
|
|
✔️ Deploy Preview for paste-docs ready! 🔨 Explore the source changes: b83ef38 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-docs/deploys/61670c9843783300070f3d98 😎 Browse the preview: https://deploy-preview-1922--paste-docs.netlify.app |
|
Size Change: +4 B (0%) Total Size: 529 kB
ℹ️ View Unchanged
|
andioneto
force-pushed
the
fix/combobox-virtualization-sizing
branch
from
cf94a17
to
14128a1
Compare
andioneto
commented
Oct 7, 2021
|
|
||
| const mockMeasureRef = jest.fn(); | ||
|
|
||
| jest.mock('react-virtual', () => { |
There was a problem hiding this comment.
👂 LMK if this isn't clear, but basically I mocked the ref here as a backdoor way to "spy" on the ref being used. I implemented it with itself because I wanted to also confirm that it correctly sets the height and ARIA attributes on virtualized items.
andioneto
commented
Oct 7, 2021
| @@ -0,0 +1,198 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
ℹ️ This is in it's own test file because I wanted to mock part of the return value from the useVirtual hook from react-virtual. It's very possible to "turn off" or reset mocks within a single test file, but I felt this would be less confusing.
SiTaggart
reviewed
Oct 8, 2021
SiTaggart
reviewed
Oct 8, 2021
packages/paste-core/components/combobox/__tests__/customization.spec.tsx
Show resolved
Hide resolved
SiTaggart
reviewed
Oct 8, 2021
SiTaggart
reviewed
Oct 8, 2021
packages/paste-core/components/combobox/__tests__/virtualization.spec.tsx
Show resolved
Hide resolved
|
This has seemed to introduce a weird bug with scrolling. If I scroll the list, and move my mouse out of the listbox, it resets the scroll position to the top of the virtual list. Browser metadata |
SiTaggart
reviewed
Oct 8, 2021
packages/paste-core/components/combobox/__tests__/virtualization.spec.tsx
Outdated
Show resolved
Hide resolved
gloriliale
reviewed
Oct 8, 2021
packages/paste-core/components/combobox/__tests__/virtualization.spec.tsx
Outdated
Show resolved
Hide resolved
gloriliale
reviewed
Oct 8, 2021
packages/paste-core/components/combobox/__tests__/virtualization.spec.tsx
Outdated
Show resolved
Hide resolved
andioneto
force-pushed
the
fix/combobox-virtualization-sizing
branch
from
532e74a
to
327ad66
Compare
|
Demo or the scroll behaviour. I don't think it's a huge deal, its definitely better than it was. Browser metadata |
andioneto
force-pushed
the
fix/combobox-virtualization-sizing
branch
from
ecaa331
to
01c278c
Compare
SiTaggart
approved these changes
Oct 12, 2021
nkrantz
approved these changes
Oct 12, 2021
| @@ -45,7 +56,7 @@ const ComboboxToTest = ({element = 'COMBOBOX'}): React.ReactElement => ( | |||
|
|
|||
| describe('Combobox data-paste-element attributes', () => { | |||
| it('should set the correct attributes on all Combobox components', () => { | |||
| const {container} = render(<ComboboxToTest />); | |||
| const {container} = render(<ComboboxToTest />, {wrapper: initCustomizationWrapper()}); | |||
| @@ -50,23 +51,25 @@ const smallGroupedItems = [ | |||
| {label: 'Design Tokens'}, | |||
| ]; | |||
|
|
|||
| const ThemeWrapper: RenderOptions['wrapper'] = ({children}) => ( | |||
There was a problem hiding this comment.
TY for explaining the differences between these wrappers yesterday!
andioneto
force-pushed
the
fix/combobox-virtualization-sizing
branch
from
4c1650a
to
b83ef38
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes bug where virtualized combobox options are not correctly resized, and adds virtualization for comboboxes that use an option template 🥳
Summary of changes
measureReffrom the return ofuseVirtualis passed as a configuration options to thegetItemPropsgetter function (Downshift.js)