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

[Feature Enhancement] Improved Emoji Support #930

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

felangel
Copy link
Contributor

@felangel felangel commented Oct 4, 2019

Summary

The existing emoji plugin is lacking support for lots of emojis. I have modified the emoji plugin to source emoji data from https://raw.githubusercontent.com/liguoqinjim/github_emoji/master/github_all/README.md instead.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:


  • DO NOT include files inside lib directory.

@felangel felangel changed the title improve emoji support [Feature Enhancement] Improved Emoji Support Oct 4, 2019
@felangel
Copy link
Contributor Author

felangel commented Oct 4, 2019

@timaschew, @jthegedus can you take a look?

@jingsam
Copy link
Contributor

jingsam commented Oct 8, 2019

@felangel thanks for your PR. The PR looks for good for me. It would be better if you just extend allGithubEmoijs varibables, not totally rewrite the whole file.

@felangel
Copy link
Contributor Author

@jingsam can you give an example of how you'd like me to structure it?

@jingsam
Copy link
Contributor

jingsam commented Oct 10, 2019

Just add missing emoijs in

const AllGithubEmoji = [
  ...
]

@felangel
Copy link
Contributor Author

felangel commented Oct 10, 2019

AllGithubEmoji was just an array of emoji id strings and the way they were being resolved was directly via the emojis endpoint.

The approach in the PR is to use the unicode endpoint and be able to just update the mapping of emoji id to image url. I'm not sure what you mean by just add the missing emojis since the whole data structure is different.

Update: I initially tried adding missing emojis to AllGithubEmoji like brazil (for example) but they don't get resolved properly which is why I refactored the whole approach.

@jingsam
Copy link
Contributor

jingsam commented Oct 10, 2019

I thought it would be better just use Github Emoji API https://api.github.com/emojis

@felangel
Copy link
Contributor Author

@jingsam sounds good. I've updated the PR 👍

@jingsam
Copy link
Contributor

jingsam commented Oct 10, 2019

LGTM @felangel thanks for your PR. Ops, it seems that I don't have the permission to merge this PR. @jthegedus How do you think about it?

@fauxpark fauxpark mentioned this pull request Oct 14, 2019
13 tasks
@timaschew
Copy link
Member

We had an discussion about emojis here: #779

@felangel please verify if this PR is aligned with the final agreement in #779

@felangel
Copy link
Contributor Author

felangel commented Oct 15, 2019

@timaschew i believe it aligns with #779 but I just added missing emoji names and left the lookup as is. It’s still using https://api.github.com/emojis to map the emoji name to the unicode url. 👍 There’s still additional work needed to automate this lookup.

Edit: @timaschew thoughts?

@felangel felangel self-assigned this Oct 15, 2019
@anikethsaha
Copy link
Member

+1 for this

@felangel
Copy link
Contributor Author

@anikethsaha if you're okay with the changes, I think we can merge this

@anikethsaha
Copy link
Member

I think we should wait for couple of days and we can merge this.

cc @timaschew @jingsam /

@timaschew
Copy link
Member

I've revisited this PR and #779.
This PR is basically just a one time fix. But let's merge ;)

@felangel felangel merged commit d9ec246 into docsifyjs:develop Nov 22, 2019
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.

4 participants