-
Notifications
You must be signed in to change notification settings - Fork 15
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
[CPT-1480] Prevent multiple Select from closing when search was clicked #4101
[CPT-1480] Prevent multiple Select from closing when search was clicked #4101
Conversation
🦋 Changeset detectedLatest commit: d9d5ab4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Greetings from FX team, @toptalwadiibasmi 👋 Thank you so much for contributing 🙇 We have got high priority ticket generated on our Kanban board so we will do our best to make your experience supreme! What's next? We will collaborate using this workflow. For you this practically means making sure DONE criteria is met and responding promptly to code review comments 😉 🙏 please, help us improve, rate your contributing experience after merge |
@toptal-anvil ping reviewers |
@toptalwadiibasmi Thank you for contribution! Can you please double-check with @whymancreates if this is a correct behaviour for Select component in #-base-core? The change does seem logical, basically we have the same behavior for multiple options select independently if the search is activated
|
@toptalwadiibasmi For example, it is not clear if we should leave the search term in input after clicking on one of the search results – right now the input is reset, not sure if this behavior is okay from point of view of Design Team Recording from pull request temploy Kapture.2024-01-17.at.11.11.35.mp4 |
@whymancreates What do you think of this behavior? The current implementation resets the search upon choosing an item. Should we retain the search term or keep it? |
@sashuk & @toptalwadiibasmi, me and @whymancreates had chance to review the tickets in terms of BASE compatibility (when the ticket was first created). And we have agreed on they are acceptable. |
Thank you @rasitozcan! @toptalwadiibasmi Can you please finish the pull request description? |
In a multiselect I agree to retain the search term in the input box and keep the result list even after selection. |
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.
Code looks good, works as expected, tests were adjusted
Error in Jest tests https://github.com/toptal/picasso/actions/runs/7544801668/job/20538958946?pr=4101#step:15:328 should be easy to fix
@toptal-anvil ping reviewers |
|
Should I take any action to get the Happo's check successful? |
8f1520f
to
faf4525
Compare
@toptalwadiibasmi Visual test is not related, I've rebased the pull request to rerun the test |
faf4525
to
abc9238
Compare
@toptal-bot run package:alpha-release |
I am unable to run the |
@toptal-bot run package:alpha-release |
Your alpha package is ready 🎉 |
@toptalwadiibasmi There was caching issue, packages are generated now 👍 |
Thanks @sashuk ! |
abc9238
to
b023176
Compare
@toptal-anvil ping reviewers |
b023176
to
06050b1
Compare
Co-authored-by: Rasit Ozcan <[email protected]>
CPT-1480
Description
The select input gains focus after clicking on a menu item only when the select is not multiple. This prevents it from closing on multiple selections when the user clicks/focuses on the search input.
How to test
Select (multiple)
Select (single)
Development checks
props
in component with documentationexamples
for componentBreaking change
PR commands
List of available commands:
@toptal-bot run package:alpha-release
- Release alpha version@toptal-anvil ping reviewers
- Ping FX team for reviewPR Review Guidelines
When to approve? ✅
You are OK with merging this PR and
nit:
to your comment. (ex.nit: I'd rename this variable from makeCircle to getCircle
)When to request changes? ❌
You are not OK with merging this PR because
When to comment (neither ✅ nor ❌)
You want your comments to be addressed before merging this PR in cases like:
How to handle the comments?