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 for clicking outside open Select closing modal #2919

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

macintoshhelper
Copy link
Contributor

@macintoshhelper macintoshhelper commented Jun 25, 2024

Why does this PR exist?

Closes #2885

What does this pull request do?

Testing this change

  • Create a new token
  • Under Modify, create a new entry, click the Select box and try clicking anywhere outside it
  • The modal should not be closed
  • Repeat for Composition

Additional Notes (if any)

For testing locally, it is necessary to clear node_modules and do a clean yarn install.

In the repository root directory:

rm -rf node_modules
yarn
Kapture.2024-06-25.at.13.17.38.mp4

Copy link

changeset-bot bot commented Jun 25, 2024

⚠️ No Changeset found

Latest commit: e4345e5

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

package.json Outdated Show resolved Hide resolved
yarn.lock Outdated
"@radix-ui/react-use-escape-keydown" "1.0.3"

"@radix-ui/react-dismissable-layer@1.0.5":
"@radix-ui/react-dismissable-layer@1.0.3", "@radix-ui/react-dismissable-layer@1.0.5":
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that there's still 2 subdependencies with different versions name, is troublesome, no? Although it's only resolving to 1.0.5 😄 Anyways, I tried with only:

"resolutions": {
    "react": "^18",
    "@radix-ui/react-dismissable-layer": "1.0.5"
  },

and it also did the trick.

Tested on the composition token creation modal, and it works well 👌

@cuserox
Copy link
Contributor

cuserox commented Jun 25, 2024

I'm wary of other multiple versions that have appeared in the yarn.lock file, such as:

Do you think this is due to the pinned dependency of "@radix-ui/react-dismissable-layer": "1.0.5"?

If so, this should be carefully tested beforehand. Next option on the table is to upgrade all linked up radix-ui packages (Dialog & Select) and hope the latest versions work together nicely.

What does the team think? CC @macintoshhelper @six7 @robinhoodie0823

For reference, my previously closed PR was another 'fix' option which involved manipulating the DOM, the caveat was that the Select lost its hover / pointer effects: #2892

Depends on the annoyance of the users as per the initial report (linked here and @SamIam4Hyma 🙏)

@six7
Copy link
Collaborator

six7 commented Jun 26, 2024

I'm wary of other multiple versions that have appeared in the yarn.lock file, such as:

Do you think this is due to the pinned dependency of "@radix-ui/react-dismissable-layer": "1.0.5"?

If so, this should be carefully tested beforehand. Next option on the table is to upgrade all linked up radix-ui packages (Dialog & Select) and hope the latest versions work together nicely.

What does the team think? CC @macintoshhelper @six7 @robinhoodie0823

For reference, my previously closed PR was another 'fix' option which involved manipulating the DOM, the caveat was that the Select lost its hover / pointer effects: #2892

Depends on the annoyance of the users as per the initial report (linked here and @SamIam4Hyma 🙏)

@macintoshhelper can you do a check of various modals / dropdowns in the plugin to ensure that we didnt regress? if so i'd say that'd be enough of a check given we'd want to upgrade radix packages soon.

otherwise 👍 from my side

Copy link
Contributor

@cuserox cuserox left a comment

Choose a reason for hiding this comment

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

Tested on the 3 components that have <Select>s inside <Modal>s, and they look good!

@macintoshhelper macintoshhelper merged commit 8718d4a into release-139 Jun 26, 2024
7 of 8 checks passed
@macintoshhelper macintoshhelper deleted the fix/select-click-closes-modal branch June 26, 2024 12:23
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.

3 participants