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): controlled combobox clear #18041

Merged

Conversation

Neues
Copy link
Contributor

@Neues Neues commented Nov 11, 2024

Closes #18027

  • update the way onChange is called in comboBox to prevent duplicate calls
  • add unit test cases for controlled combobox managed by parent component
  • fix console errors related to missing onChange prop from ComboBox.mdx
  • fix console errors for components using WithLayer related to wrong proptypes
    Screenshot 2024-11-14 at 11 15 19

Testing / Reviewing

  • add log to onChange in fullyControlled combobox in ComboBox.stories.js
  • select different options to see onChange fires only once
  • select the same option to see that onChange does not fire
  • add log to onChange other ComboBox stories to ensure it is firing the appropriate amount of times
  • verify console errors are no longer present

* update the way onChange is called in comboBox to prevent unwanted calls
* add unit test cases for controlled combobox managed by parent component
@Neues Neues requested a review from a team as a code owner November 11, 2024 20:04
Copy link

netlify bot commented Nov 11, 2024

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

Name Link
🔨 Latest commit e72e48e
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/6735da161977b0000811407c
😎 Deploy Preview https://deploy-preview-18041--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 Nov 11, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit e72e48e
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6735da161977b0000811407a
😎 Deploy Preview https://deploy-preview-18041--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 Nov 11, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e72e48e
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6735da16d85045000800e865
😎 Deploy Preview https://deploy-preview-18041--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 Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.15%. Comparing base (ad23c63) to head (e72e48e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #18041   +/-   ##
=======================================
  Coverage   82.14%   82.15%           
=======================================
  Files         404      404           
  Lines       14117    14121    +4     
  Branches     4434     4442    +8     
=======================================
+ Hits        11597    11601    +4     
+ Misses       2359     2358    -1     
- Partials      161      162    +1     

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

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking a second look at this!

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.

Hey @Neues Thanks for opening that!
I'll close my PR that has a similar fix. I'll fix the format in this one, it is the same error I got.

guidari and others added 3 commits November 13, 2024 14:53
* fix withLayer proptypes
* add required onChange prop to examples in ComboBox.mdx
@guidari guidari added this pull request to the merge queue Nov 14, 2024
Merged via the queue into carbon-design-system:main with commit 1009bab Nov 14, 2024
40 checks passed
onChange({ selectedItem: newSelectedItem });
}
if (
type === '__function_select_item__' ||

Choose a reason for hiding this comment

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

Should use enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pull request is already merged, and a fix for this has also been merged, see #18145

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]: Controllled combobox fires onchange twice when input field is cleared
4 participants