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

Emoji fallback shouldn't apply styling #157 #158

Merged
merged 3 commits into from Jan 15, 2018

Conversation

ajbeaven
Copy link

@ajbeaven ajbeaven commented Jan 9, 2018

My use of the fallback mechanism just added is the same as was mentioned in the linked issue.

<Emoji
  set={'messenger'}
  emoji={'shrug'}
  size={24}
  fallback={(emoji) => {
    return `:${emoji.short_names[0]}:`
  }}
/>

In a case where the fallback is utilised, I'd prefer that styling wasn't applied.

Of course, feel free to close this pull request if there are other reasons that the font-size needs to be maintained!

@ajbeaven
Copy link
Author

ajbeaven commented Jan 9, 2018

Ah, dang, it's because the fallback is rendered inside the emoji <span>. How about this...?

@EtienneLem
Copy link
Member

I see, it does make sense. Anyone wanting to apply a font-size (or any other style) can do so by styling whatever fallback returns (i.e. it can be JSX).

I think with that version we could remove the fallback: emoji => {} defaultProps. It really is my fault, it shouldn’t have been defined to being with, but we might as well make it part of that PR.

@ajbeaven
Copy link
Author

Sure thing! I've now removed the fallback from defaultProps (do participants get notifications of commits being added to PRs?).

@EtienneLem
Copy link
Member

Sweet, thanks!

(do participants get notifications of commits being added to PRs?).

I do via email. Here’s my GitHub notifications settings and I believe these are the defaults.

screen shot 2018-01-10 at 4 51 43 pm

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