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

feat!: Make DropdownAPI consistent and fix keyboard handling #843

Merged
merged 3 commits into from Mar 1, 2021

Conversation

jquense
Copy link
Member

@jquense jquense commented Jul 21, 2020

Also removes the need for a wrapping element

@jquense jquense requested a review from taion July 21, 2020 18:53
@jquense jquense changed the title feat: Make DropdownAPI consistent and fix keyboard handling feat!: Make DropdownAPI consistent and fix keyboard handling Jul 21, 2020
}

function useRefWithUpdate() {
const forceUpdate = useForceUpdate();
Copy link
Member

Choose a reason for hiding this comment

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

question: do these get properly enqueued/deduped, or could we end up with a double-rerender?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fairly sure it does, maybe not a big deal either way idk

@jquense
Copy link
Member Author

jquense commented Jul 24, 2020

I realized recently that we don't handle escape inside of a modal for dropdowns which leads to both firing. Bootstrap calls preventDefault and stopPropagation for those, but that's tricky here given that this now fires after react sees the event.

Should try and attach the events in react via props? I was trying to avoid extra event merging needed by users, but maybe that's ok here? Alternatively i could do the capture phase tagging etc

@taion
Copy link
Member

taion commented Jul 24, 2020

ohh that's unfortunate.

these don't even both go through root close wrapper, right?

i guess we could have some shared WeakMap or something there to ensure events are only processed once? would also address #836

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.

None yet

2 participants