Skip to content

Fix issues with select dropdown#1378

Merged
mrubens merged 1 commit intomainfrom
select_dropdown_cleanup
Mar 4, 2025
Merged

Fix issues with select dropdown#1378
mrubens merged 1 commit intomainfrom
select_dropdown_cleanup

Conversation

@mrubens
Copy link
Collaborator

@mrubens mrubens commented Mar 4, 2025

Context

Found a couple issues when playing around with #1329

  • If you went to another tab when the dropdown was open, it would jump to the top left
  • Clicking outside didn't close the dropdowns
  • We still had logic looking for a fixed string separator

Implementation

  • Fixed the main issue by specifying the portal to use
  • Added onEscapeKeyDown and onInteractOutside handlers
  • Removed the old code

Screenshots

Before:

Screenshot 2025-03-04 at 3 23 19 PM

After: Goes away


Important

Fixes SelectDropdown issues by adding event handlers for closing dropdown and removing outdated string separator logic.

  • Behavior:
    • Adds onEscapeKeyDown and onInteractOutside handlers in select-dropdown.tsx to close dropdown on escape key or outside interaction.
    • Removes logic for string separator handling in select-dropdown.tsx.
  • Tests:
    • Adds test for controlled open state in select-dropdown.test.tsx.
    • Removes test for string separator in select-dropdown.test.tsx.
  • Misc:
    • Uses useEffect to set portalContainer for dropdown rendering in select-dropdown.tsx.

This description was created by Ellipsis for 74d52aa. It will automatically update as commits are pushed.

@mrubens mrubens requested a review from cte as a code owner March 4, 2025 20:25
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Mar 4, 2025
@mrubens mrubens force-pushed the select_dropdown_cleanup branch from 6a1531f to 2e4053c Compare March 4, 2025 22:34
@changeset-bot
Copy link

changeset-bot bot commented Mar 4, 2025

⚠️ No Changeset found

Latest commit: 74d52aa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 4, 2025
@mrubens mrubens force-pushed the select_dropdown_cleanup branch from 2e4053c to 74d52aa Compare March 4, 2025 22:35
expect(trigger.classList.toString()).toContain("custom-trigger-class")
})

it("ensures open state is controlled via props", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The new test "ensures open state is controlled via props" only checks the presence of elements without simulating any interactions that change the open state. Consider adding tests to simulate opening the dropdown and then triggering the escape key and outside clicks to ensure the onEscapeKeyDown and onInteractOutside handlers properly close the dropdown.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 4, 2025
@mrubens mrubens merged commit 9ffd389 into main Mar 4, 2025
11 checks passed
@mrubens mrubens deleted the select_dropdown_cleanup branch March 4, 2025 22:43
ipattis pushed a commit to ipattis/roo-code that referenced this pull request Mar 15, 2025
…ings-button

Add advanced settings button / Add setting to exclude MCP prompting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants