Skip to content

fix: click callback inside SelectItem shouldn't be omitted #3472

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: click callback inside SelectItem shouldn't be omitted #3472

wants to merge 1 commit into from

Conversation

himself65
Copy link

@himself65 himself65 commented Apr 15, 2025 •
edited
Loading

Fixes: #3460

Wrap with startTransition will allow consecutive logic can be handled correctly

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Apr 15, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: a8fc7fe

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

@himself65 himself65 force-pushed the link-inside-select branch 3 times, most recently from 7411555 to e2ca631 Compare April 15, 2025 04:55
@himself65 himself65 force-pushed the link-inside-select branch from cd6dcec to a8fc7fe Compare April 15, 2025 04:57
SukkaW
SukkaW reviewed Apr 15, 2025
View reviewed changes
packages/react/select/src/select.tsx
Comment on lines +1272 to +1278
const startTransition = React.startTransition
? React.startTransition
: window.requestAnimationFrame;
startTransition(() => {
context.onValueChange(value);
context.onOpenChange(false);
});
Copy link

Choose a reason for hiding this comment

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

This seems wrong. The value change handler should have the highest priority (higher than render/paint) to quickly respond to the user interaction. Thus, you can't use requestAnimationFrame here.

The startTransition also feels wrong, as this schedules the value change to the next re-render.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for feedback. What s your solution for solving anchor element here?

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

@SukkaW SukkaW SukkaW left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<a/> element in SelectItem is not accessable
2 participants
@himself65 @SukkaW