Skip to content

Fix: the ability to use a custom button via buttonRef prop was broken.#45

Open
some1-in-here wants to merge 2 commits into
cesarwbr:masterfrom
some1-in-here:fix-buttonRef-prop
Open

Fix: the ability to use a custom button via buttonRef prop was broken.#45
some1-in-here wants to merge 2 commits into
cesarwbr:masterfrom
some1-in-here:fix-buttonRef-prop

Conversation

@some1-in-here
Copy link
Copy Markdown

It was impossible to use custom buttonRef.
Moreover, the buttonRef has been combined with the EmojiPicker component in the React.portal. And now, we have two components independent of each other.

If any changes are required, I will be happy to make them.

@AJBrownson
Copy link
Copy Markdown

@cesarwbr Would be nice if this pull request was merged

@cesarwbr
Copy link
Copy Markdown
Owner

Thanks for identifying this bug — the appendChild DOM mutation inside React effects is definitely an anti-pattern. A few things to address before merging:

Removing the ReactDOM.createPortal() entirely may break the buttonElement (non-ref) code path — could you verify?
The native addEventListener will pass a native MouseEvent to toggleShowPicker, but calcTopPosition uses event.currentTarget which may behave differently with native vs synthetic events.
typeButton has no initial state, causing a brief flash on first render.
The direction is right though — would love to see an updated version addressing these.

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