fix: fixed the onchange firing undefined #19085

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.

Sign up for GitHub

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

Jump to bottom

Merged

Gururajj77 merged 14 commits into main from combobox-onchange-again

Apr 23, 2025

Contributor

Gururajj77 commented Apr 10, 2025

Closes #18361

bug fixes on the combobox

silenced the onChange firing with undefined
now, mouse and keyboard interactions won't fire onChange, if the user selects an already selected it.
this holds good for both with and without allowCustomValue prop.

Changelog

Changed

added isManualClearingRef to keep track of the user clearing the items using the crossmark (X).
rewrote the InputKeydownEnter case in the reducer, to handle the selections accordingly for both scenarios of with and without allowCustomValue
added the undefined guards in the onStateChange to make sure it doesn't fire onChange with undefined which triggers other parts of the code.
rewrote the clearSelection function that is in the <ListBoxSelection /> to fire onChange with null.

Testing / Reviewing

there are two test stores, "Undefined Checks Testing" and "Undefined Checks with Allow Custom Value"
In both of the test stories check for these interactions:
- Open the console
- select and item using keyboard
- see that console logs with that selected item in selectedItem: <your-selected-item>
- select the same item again in the list
- see that onChange doesn't fire again with that same item
- select another item
- see that onchange fires with that new selected item.
- perform the same interactions using the mouse
- play around the buttons to test the fully controlled version also.
Make sure that in the two test stories
- In the "Undefined Checks Testing" story, when you type custom value and close the combobox, it clears off that input
- In the "Undefined Checks with Allow Custom Value", when you type custom value and close the combobox, the input stays the same without clearing the value, and an onChange is fired with { selectedItem: null, inputValue: "<your-custom-value"
Delete the two test stories after approval, before merging.


          fix: fixed the onchange firing undefined, again

2c89470

Gururajj77 requested review from a team as code owners

April 10, 2025 09:04

Gururajj77 requested review from 2nikhiltom and tay1orjones

April 10, 2025 09:04

Gururajj77 added the status: ready for review 👀 label


          fix: added test story again

0453b31

netlify bot commented Apr 10, 2025

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

Name	Link
🔨 Latest commit	`2c89470`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-web-components/deploys/67f789aefd142000085a80f6
😎 Deploy Preview	https://deploy-preview-19085--v11-carbon-web-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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


          Merge branch 'main' into combobox-onchange-again

e966907

netlify bot commented Apr 10, 2025

✅ Deploy Preview for v11-carbon-react ready!

Name	Link
🔨 Latest commit	`2c89470`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/67f789ae985e4b0008e02ae2
😎 Deploy Preview	https://deploy-preview-19085--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 10, 2025

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

Name	Link
🔨 Latest commit	`0453b31`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-web-components/deploys/67f789f2088fc40008d4b478
😎 Deploy Preview	https://deploy-preview-19085--v11-carbon-web-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 10, 2025 •

edited

Loading

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

Name	Link
🔨 Latest commit	`9e3f822`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-web-components/deploys/68087efc768ff500080e1931
😎 Deploy Preview	https://deploy-preview-19085--v11-carbon-web-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Apr 10, 2025 •

edited

Loading

✅ Deploy Preview for v11-carbon-react ready!

Name	Link
🔨 Latest commit	`9e3f822`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/68087efcdbe2150008862880
😎 Deploy Preview	https://deploy-preview-19085--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

codecov bot commented Apr 10, 2025 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.41%. Comparing base (fd1dafb) to head (9e3f822).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #19085      +/-   ##
==========================================
+ Coverage   84.38%   84.41%   +0.03%     
==========================================
  Files         386      385       -1     
  Lines       14540    14557      +17     
  Branches     4802     4809       +7     
==========================================
+ Hits        12269    12288      +19     
+ Misses       2112     2109       -3     
- Partials      159      160       +1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Gururajj77 changed the title ~~fix: fixed the onchange firing undefined, again~~ fix: fixed the onchange firing undefined

adamalston reviewed

View reviewed changes

packages/react/src/components/ComboBox/ComboBox.stories.js Outdated Show resolved Hide resolved

packages/react/src/components/ComboBox/ComboBox.tsx Outdated Show resolved Hide resolved

packages/react/src/components/ComboBox/ComboBox.tsx Outdated Show resolved Hide resolved

packages/react/src/components/ComboBox/ComboBox.tsx Outdated Show resolved Hide resolved

Gururajj77 and others added 3 commits

April 15, 2025 15:35


          Update packages/react/src/components/ComboBox/ComboBox.tsx

b2aad94

Co-authored-by: Adam Alston <[email protected]>


          Update packages/react/src/components/ComboBox/ComboBox.stories.js

c97e9e1

Co-authored-by: Adam Alston <[email protected]>


          Update packages/react/src/components/ComboBox/ComboBox.tsx

735eff4

Co-authored-by: Adam Alston <[email protected]>

Contributor

alewitt2 commented Apr 17, 2025

any updates on this fix? I'm currently blocked by this bug

Member

tay1orjones commented Apr 17, 2025

@Gururajj77 were you able to find a different approach that doesn't use a setTimeout?

Also a reminder that this needs tests covering the interactions you describe in the PR body and in the originally linked issue.

Gururajj77 added 2 commits

April 21, 2025 14:51


          fix: changed from settimeout to combo of usestate and useeffect:

ebff3f5


          fix: added tests for onchange

ccee66f

Contributor Author

Gururajj77 commented Apr 21, 2025

@Gururajj77 were you able to find a different approach that doesn't use a setTimeout?

Also a reminder that this needs tests covering the interactions you describe in the PR body and in the originally linked issue.

Yes, I've figured out a way to make it work without setTimeout. It's using a combination of state and ref. Added some tests for them to make sure the functionality is covered.


          fix: added tests for allowcustomvalue

2ca91b5

Contributor

adamalston commented Apr 21, 2025 •

edited

Loading

When I revert all of the code changes, the tests still pass. Shouldn't they be failing?

git checkout combobox-onchange-again
git checkout main packages/react/src/components/ComboBox/ComboBox.tsx
yarn test packages/react/src/components/ComboBox/ComboBox-test.js

Gururajj77 and others added 3 commits

April 22, 2025 18:35


          Merge branch 'main' into combobox-onchange-again

ac4e428


          test: fixed the tests to make sure we are capturing the problem

0b5baa8


          Merge branch 'main' into combobox-onchange-again

0804ca8

Gururajj77 requested a review from adamalston

April 22, 2025 17:21

Contributor Author

Gururajj77 commented Apr 22, 2025

When I revert all of the code changes, the tests still pass. Shouldn't they be failing?
git checkout combobox-onchange-again
git checkout main packages/react/src/components/ComboBox/ComboBox.tsx
yarn test packages/react/src/components/ComboBox/ComboBox-test.js

Rewritten the tests, and added cases for both cases where the items are array of strings, and array of objects, to make sure all cases are covered.

Gururajj77 mentioned this pull request

[Bug]: Synchronization between combobox's External and Internal (controlled and uncontrolled) selection is broken. #19206

Open

2 tasks

This comment was marked as off-topic.

Sign in to view

tay1orjones approved these changes

View reviewed changes

Member

tay1orjones left a comment

I moved my previous findings to that new issue

tay1orjones added the status: one more review 👀 label

adamalston reviewed

View reviewed changes

packages/react/src/components/ComboBox/ComboBox-test.js Outdated Show resolved Hide resolved

packages/react/src/components/ComboBox/ComboBox.tsx Outdated Show resolved Hide resolved

Gururajj77 added 2 commits

April 23, 2025 11:06


          chore: reused component for tests

6c3a01b


          refactor: onstatechange improvements

9e3f822

2nikhiltom approved these changes

View reviewed changes

Gururajj77 requested a review from adamalston

April 23, 2025 14:08

Gururajj77 added this pull request to the merge queue

Merged via the queue into main with commit 3e3a1a9

37 checks passed

Gururajj77 deleted the combobox-onchange-again branch

April 23, 2025 14:34

Contributor

carbon-automation bot commented Apr 23, 2025

Hey there! v11.81.0 was just released that references this issue/PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: one more review 👀 status: ready for review 👀