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.

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

feat(combobox): expose downshift actions through downshiftActions ref prop #17118

Merged

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented Aug 7, 2024

Closes #17027

Follow up to #17089, this ensures that downshift action functions can be called from within parent components using Combobox.

Click to expand notes on compatibility

The functionality behind both downshiftProps and downshiftActions isn't guaranteed and is subject to change. This PR patches a functionality mismatch internal to downshift. It does not cover all previous action:

Actions previously available Actions made available in this PR
clearSelection
clearItems
closeMenu closeMenu
openMenu openMenu
selectHighlightedItem
selectItem selectItem
selectItemAtIndex
setHighlightedIndex setHighlightedIndex
toggleMenu toggleMenu
reset reset
setItemCount
unsetItemCount
setState
setInputValue

If a consumer was using an action that is no longer available, such as setState, a functional equivalent can usually be found. This will require a code change in consuming projects.

- stateAndHelpers.setState({ inputValue: 'Item 1' });
+ downshiftActions.current.setInputValue('Item 1');

Changelog

New

  • Add downshiftActions prop to Combobox

Changed

  • Update Combobox documentation for downshiftActions and downshiftProps
  • Improve disclaimer warnings on both these props across all relevant downshift components
  • Fix a bug in Combobox where the forwarded ref was not properly being applied through incorrect mergeRefs usage

Testing / Reviewing

The new test story in storybook implements the same logic referenced in the original issue report's stackblitz reproduction.

Go to the test story and validate base functionality. There should be no other changes to Combobox stories.

  • ⚠️ Need to remove test story before merging

@tay1orjones tay1orjones requested a review from a team as a code owner August 7, 2024 19:35
@tay1orjones tay1orjones requested review from emyarod and guidari August 7, 2024 19:35
Copy link

netlify bot commented Aug 7, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 59b44dd
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/66b5648736cdd90008c298b6
😎 Deploy Preview https://deploy-preview-17118--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Aug 8, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 59b44dd
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66b56487ebe36a0008795dbb
😎 Deploy Preview https://deploy-preview-17118--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tay1orjones tay1orjones requested a review from a team as a code owner August 8, 2024 19:30
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

the test story crashes when I interact with it

ReferenceError: component is not defined
    at Object.onStateChange (https://deploy-preview-17118--v11-carbon-react.netlify.app/ComboBox-stories.5248b449.iframe.bundle.js:1:29192)
    at callOnChangeProps (https://deploy-preview-17118--v11-carbon-react.netlify.app/1714.6bf0042b.iframe.bundle.js:1:31833)
    at https://deploy-preview-17118--v11-carbon-react.netlify.app/1714.6bf0042b.iframe.bundle.js:1:33956
    at Qj (https://deploy-preview-17118--v11-carbon-react.netlify.app/7319.005706cc.iframe.bundle.js:528:591606)
    at Hk (https://deploy-preview-17118--v11-carbon-react.netlify.app/7319.005706cc.iframe.bundle.js:528:611538)
    at Wk (https://deploy-preview-17118--v11-carbon-react.netlify.app/7319.005706cc.iframe.bundle.js:528:610306)
    at Pk (https://deploy-preview-17118--v11-carbon-react.netlify.app/7319.005706cc.iframe.bundle.js:528:610376)
    at Ek (https://deploy-preview-17118--v11-carbon-react.netlify.app/7319.005706cc.iframe.bundle.js:528:604186)
    at jg (https://deploy-preview-17118--v11-carbon-react.netlify.app/7319.005706cc.iframe.bundle.js:528:545214)
    at https://deploy-preview-17118--v11-carbon-react.netlify.app/7319.005706cc.iframe.bundle.js:528:601606

but changing component to this seems to fix the story and it looks like feature is working as expected

image

@tay1orjones
Copy link
Member Author

@emyarod ah my bad - I copied code from the stackblitz reproduction and forgot to update a couple of the usages from component to changes 🤦 It's fixed now

Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

Cool! The stackblitz from this issue can console.log now
LGTM! 🚀

@tay1orjones
Copy link
Member Author

For posterity, here's the three approaches that I explored:

  1. A ref prop. The consumer passes in a ref that we mutate to contain the actions. It's what's in this PR.

  2. A callback function prop. I don't think this would provide any benefit over using a ref. In both instances it's possible for the actions to be initially undefined, and they are only updated if the action functions change (since either approach is contained within a useEffect). Consumers would also need to store the values returned in the callback in state.

  3. Use useImperativeHandle on forwardRef to expose the action functions. This would change the signature of the forwarded ref and would break anyone relying on ref.current to be a dom node. We would still expose the node in addition to the action functions, but it would require anyone using the ref to update their code. ref.current => ref.current.inputNode.

Option 1 patches the bug, avoids consumers having to store anything in state, and doesn't introduce problems for projects using the forwardRef.

@guidari guidari enabled auto-merge August 9, 2024 11:11
@tay1orjones tay1orjones disabled auto-merge August 9, 2024 12:57
@tay1orjones tay1orjones merged commit 748400f into carbon-design-system:main Aug 9, 2024
22 checks passed
tay1orjones added a commit to tay1orjones/carbon that referenced this pull request Aug 9, 2024
… prop (carbon-design-system#17118)

* chore(wip): expose actions using downshiftActions ref prop

* docs(combobox): add downshiftActions

* fix(combobox): fix typings

* chore: update snaps

* fix: test story

* fix(a11y): lock a11y checker to previous ruleset
@tay1orjones tay1orjones mentioned this pull request Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: ComboBox downshiftProps missing reference to Downshift component where state change can be done
3 participants