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

runfix: Address emoji picker styles #14037

Merged
merged 2 commits into from
Nov 3, 2022
Merged

runfix: Address emoji picker styles #14037

merged 2 commits into from
Nov 3, 2022

Conversation

atomrc
Copy link
Contributor

@atomrc atomrc commented Nov 3, 2022

This PR fixes the display of the emoji picker than was broken since #13493

It also modernizes the styles and migrate them to css-in-js

Before
image

After
image

@atomrc atomrc requested review from a team and otto-the-bot as code owners November 3, 2022 16:47
@@ -33,15 +34,15 @@ interface EmojiItemProps {
const EmojiItem: FC<EmojiItemProps> = ({emoji, onClick, onMouseEnter, selectedEmoji = false}) => (
<button
type="button"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arjita-mitra I think this type=button is not necessary here. Is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's only needed for input, am I correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh actually I found my answer and it is needed :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for tabIndex={0}, looking like leftovers from changing the component from div to button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh correct the tabindex is not needed, good catch 👍

Copy link
Contributor

@V-Gira V-Gira left a comment

Choose a reason for hiding this comment

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

I'll take that as an endorsment for team CSSObject

@atomrc
Copy link
Contributor Author

atomrc commented Nov 3, 2022

I'll take that as an endorsment for team CSSObject

I'm team css-in-js yes :) (also because it's the solution we already have baked in)

@atomrc atomrc enabled auto-merge (squash) November 3, 2022 16:56
@atomrc atomrc disabled auto-merge November 3, 2022 17:22
@atomrc atomrc merged commit 4ff5b23 into dev Nov 3, 2022
@atomrc atomrc deleted the runfix/emoji branch November 3, 2022 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants