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

fix(combobox): onInputChange calls #18713

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Neues
Copy link
Contributor

@Neues Neues commented Feb 26, 2025

Closes #16969

Fix defect in ComboBox causing onInputChange to be called on initial render.

Changelog

Changed

  • Remove useEffect calling onInputChange on initial render
  • Leverage useCombobox's onStateChange to call onInputChange. Note that they provide onInputValueChange. They advise to use this only when inputValue is controlled, and in our case, we only are providing the callback. Therefore I think it is ok to handle this in onStateChange.
  • Unit tests

Testing/Reviewing

yarn test packages/react/src/components/ComboBox

Observing Defect:
on main branch, add onInputChange prop to default ComboBox in ComoboBox.stories.js
ie, line 192

        onInputChange={(c) => console.log('onInputChange called with: ', c)}

run storybook

yarn storybook

http://localhost:3001/?path=/story/components-combobox--default

confirm that log shows up in console on initial render

Observing fix:

git switch combobox-onInputChange-fix

repeat steps above and confirm onInputChange is not called on initial render, and is only called on input change

* Remove useEffect calling onInputChange on initial render
* Leverage useCombobox's onStateChange to call onInputChange
* Unit tests
Copy link

netlify bot commented Feb 26, 2025

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit c30b18b
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/67bf50dee021010008637f7e
😎 Deploy Preview https://deploy-preview-18713--v11-carbon-web-components.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 Feb 26, 2025

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit c30b18b
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/67bf50de89f4d100087f78d5
😎 Deploy Preview https://deploy-preview-18713--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 Feb 26, 2025

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c30b18b
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/67bf50de3bf87900086d9372
😎 Deploy Preview https://deploy-preview-18713--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.

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.34%. Comparing base (1d57a3a) to head (c30b18b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #18713   +/-   ##
=======================================
  Coverage   84.34%   84.34%           
=======================================
  Files         408      408           
  Lines       14645    14646    +1     
  Branches     4791     4824   +33     
=======================================
+ Hits        12352    12353    +1     
  Misses       2131     2131           
  Partials      162      162           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Neues Neues marked this pull request as ready for review February 26, 2025 18:33
@Neues Neues requested review from a team as code owners February 26, 2025 18:33
@Gururajj77
Copy link
Contributor

Hey @Neues, setting up a test story would help us to test the fix. Could you do that please??

@Neues
Copy link
Contributor Author

Neues commented Feb 27, 2025

No problem @Gururajj77, comment updated with instructions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Combobox fires its onInputChange callback on initial render
2 participants