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

Change number emoji to use unicode characters #1016

Merged
merged 2 commits into from
Feb 15, 2020

Conversation

alyip98
Copy link
Contributor

@alyip98 alyip98 commented Feb 3, 2020

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Bug fix

Fixes #992

What is the rationale for this request?
The links we use for number emoji (e.g. 1️⃣ ) from the Github CDN are no longer working, as they have changed to use unicode characters.

What changes did you make? (Give an overview)
Change the number emoji used in markdown-it-emoji to use unicode characters.

Proposed commit message: (wrap lines at 72 characters)

Number emojis in Markbind use the emoji images from Github's CDN.
Github has migrated to use unicode characters for their emojis, causing
our number emojis to break.

Let's update our number emojis to use the unicode characters as well.

Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this! Just a question 🙂

@@ -1,15 +1,13 @@
// fix emoji numbers
const emojiData = require('markdown-it-emoji/lib/data/full.json');
// Extend emoji here
emojiData['zero'] = emojiData['0'] = '<img style="height: 1em;width: 1em;margin: 0 .05em 0 .1em;vertical-align: -0.1em;" src="https://assets-cdn.github.com/images/icons/emoji/unicode/0030-20e3.png">';
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we are also populating the digit key here (i.e. emojiData['1']) alongside the letter key, but we don't seem to do that in the new version. Is that intentional?

Copy link
Member

Choose a reason for hiding this comment

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

Also I strongly encourage adopting the existing code's format (i.e. assigning each individual emoji line by line). The new changes disassociates the mapping between the names and the corresponding emoji.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like we are also populating the digit key here (i.e. emojiData['1']) alongside the letter key, but we don't seem to do that in the new version. Is that intentional?

I did considering keeping the digit mapping for backwards compatibility. However, as they aren't standard emoji, and I could not find any current uses in the SE textbook and Markbind docs, I thought it would be better to remove them.


Also I strongly encourage adopting the existing code's format (i.e. assigning each individual emoji line by line). The new changes disassociates the mapping between the names and the corresponding emoji.

OK, I see your point. I will update the PR.

@yamgent yamgent modified the milestones: v2.10.0, v2.10.1 Feb 6, 2020
@yamgent yamgent merged commit e22fb28 into MarkBind:master Feb 15, 2020
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.

Emoji cdn links for markdown-it-emoji don't work
3 participants