Skip to content

[Feature Enhancement] Improved Emoji Support#930

Merged
felangel merged 1 commit intodocsifyjs:developfrom
felangel:develop
Nov 22, 2019
Merged

[Feature Enhancement] Improved Emoji Support#930
felangel merged 1 commit intodocsifyjs:developfrom
felangel:develop

Conversation

@felangel
Copy link
Contributor

@felangel felangel commented Oct 4, 2019 •
edited
Loading

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 •
edited
Loading

@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

felangel commented Oct 10, 2019

@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 •
edited
Loading

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

felangel commented Oct 10, 2019

@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
Closed
13 tasks
@timaschew
Copy link
Member

timaschew commented Oct 15, 2019

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 •
edited
Loading

@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
@felangel felangel requested a review from timaschew October 15, 2019 13:29
@anikethsaha
Copy link
Member

anikethsaha commented Nov 21, 2019

+1 for this

@felangel
Copy link
Contributor Author

felangel commented Nov 21, 2019

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

@anikethsaha
Copy link
Member

anikethsaha commented Nov 21, 2019

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

cc @timaschew @jingsam /

@timaschew
Copy link
Member

timaschew commented Nov 22, 2019

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
@fauxpark fauxpark mentioned this pull request Mar 12, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timaschew timaschew timaschew approved these changes

Assignees

@felangel felangel

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@felangel @jingsam @timaschew @anikethsaha