-
Notifications
You must be signed in to change notification settings - Fork 114
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
fix(combobox/combobox-primitive): fix multiselect example and ref merge bug #2155
Conversation
🦋 Changeset detectedLatest commit: 070a0c4 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 |
✔️ Deploy Preview for paste-docs ready! 🔨 Explore the source changes: 070a0c4 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-docs/deploys/62043712f9cb6a0008849759 😎 Browse the preview: https://deploy-preview-2155--paste-docs.netlify.app |
Size Change: +2.26 kB (0%) Total Size: 463 kB
ℹ️ View Unchanged
|
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
packages/paste-core/primitives/combobox/stories/index.stories.tsx
Outdated
Show resolved
Hide resolved
✔️ Deploy Preview for paste-theme-designer ready! 🔨 Explore the source changes: 070a0c4 🔍 Inspect the deploy log: https://app.netlify.com/sites/paste-theme-designer/deploys/620437127dcf470008ade498 😎 Browse the preview: https://deploy-preview-2155--paste-theme-designer.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just need to update the website examples to match the latest changes in the storybook file.
Test summaryRun 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 |
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
packages/paste-website/src/component-examples/ComboboxPrimitiveExamples.ts
Show resolved
Hide resolved
packages/paste-website/src/component-examples/ComboboxPrimitiveExamples.ts
Outdated
Show resolved
Hide resolved
oh my find about the website example might be bc of this that @shleewhite had said?
|
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
9b72341
to
4a61ea4
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 070a0c4:
|
@SiTaggart @TheSisb Here are some additional resources on "merging" props like refs and event handlers: |
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
f300aea
to
8152660
Compare
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
packages/paste-core/primitives/combobox/stories/mutliselect.stories.tsx
Outdated
Show resolved
Hide resolved
packages/paste-website/src/component-examples/ComboboxPrimitiveExamples.ts
Show resolved
Hide resolved
9139716
to
ff720fb
Compare
25c3cbe
to
8a900ef
Compare
packages/paste-core/components/combobox/src/extractPropsFromState.tsx
Outdated
Show resolved
Hide resolved
packages/paste-core/primitives/combobox/stories/index.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be in such a better space now. Just a couple of small things to tidy up
8a900ef
to
28dfdcb
Compare
28dfdcb
to
2743cdd
Compare
a4daa30
to
070a0c4
Compare
Description
Continuation of PR 2119 from @shleewhite.
Fix
useMultiSelectPrimitive
examplesIssue
A customer reported that our multiple selection example is not working as expected.
Cause
Downshift does not automatically "reset" the
useCombobox
selected item state. The just-selected, then removed, item cannot be selected again immediately after because it is still set as the currently selected Item.Fix
Manually reset the selected item state for the multiple selection examples.
Fix
ref
errors reported in consoleIssue
We were seeing
console.error
statements indicating that theref
properties from various prop getters were not being correctly passed to indented components. This was specifically happening for the controlled component whenever we passed our combobox its state via ourstate
property.Cause
We are currently conditionally using a "default" state for the combobox, by allowing our customers to pass in their own state that matches the API specifications of our component and the managing hook. Due to internal implementation details of the
useCombobox
hook, this caused the observed side effects.Fix
Remove default hook render if
state
property is received by the component.Summary of changes:
selectedItem
to null after selection in multi primitive storybook