Skip to content
This repository has been archived by the owner on Apr 3, 2024. It is now read-only.

Emoji in headers is stripped from anchors #128

Closed
chrisdickinson opened this issue Jan 23, 2016 · 10 comments
Closed

Emoji in headers is stripped from anchors #128

chrisdickinson opened this issue Jan 23, 2016 · 10 comments
Labels

Comments

@chrisdickinson
Copy link

Hi! I've been poking at writing some documentation, and in the course of playing around with another tool I ran into a discrepancy between how GitHub generates heading slugs vs. how npm renders them, in particular when emoji appears in the header.

I've created a test repo and published it. It appears that npm strips emoji from the anchor entirely, while GitHub includes the emoji's GH nickname in the anchor.

This isn't blocking anything for me; this issue is by way of cataloguing the differences between GH's rendering and ours — apologies if this is a known thing. If so, please don't hesitate to close this out!

Thanks, all!

@ashleygwilliams
Copy link
Contributor

thanks for reporting @chrisdickinson !

@revin
Copy link
Collaborator

revin commented Jan 28, 2016

Yes, thanks @chrisdickinson!

OK @ashleygwilliams I filed a PR on github-slugger since that's what we use to generate the slugs and it made sense to me to have the emoji code live there. It's github-slugger#2.

I'm somewhat amused that much of the time, the emojis will start as :shortcode:-style strings in the markdown, then get converted to HTML via markdown-it, then get converted back to shortcodes with the stuff I PR'd for github-slugger. But given that the alternative was to be more invasive and hackier in markdown-it, it seemed to me like the cleanest way to do it. As always, I'm totally open to critique/disagreement/reassessment.

@revin
Copy link
Collaborator

revin commented Jan 29, 2016

All right, they declined the PR and recommended we make the change on our side, so I submitted PR #133 here to do just that.

@ashleygwilliams
Copy link
Contributor

iirc github slugger was made for this repo. if they wont take it, lets fork. (will review when i am no longer on planebut that is my initial thought)

@zeke
Copy link
Contributor

zeke commented Jan 29, 2016

cc @Flet

@wooorm
Copy link

wooorm commented Jan 29, 2016

Hey all! I’m the one who “declined” / advised against the pull, not @Flet, it’s his project so if he wants he should definitely merge.

However, the reason for advising against is because the proposed solution actually breaks other things on npm. Sorry, if that didn’t come across.

Here’s some more in depth description of what I tried to say over at Flet/github-slugger#2.

There’s a different slugging happening between the following two headers on GitHub:

# 😄 - an emoji

# :smile: - a gemoji

The first’s slug is ---an-emoji, the second smile---a-gemoji.

If npm-marky converts both headings to emoji (😄), as it currently does, and github-slugger converts that back to :smile:, as proposed by the PR, the same slugs come out on npm, whereas they should be different according to GitHub.

Thus, if you want the same slugging to happen as on GitHub, github-slugger is acting as required, and marky-markdown needs a change somewhere.

Again, sorry that that didn’t come across!

@revin
Copy link
Collaborator

revin commented Jan 29, 2016

oh I gotcha, thanks for the clarification @wooorm 👍

Hmm, so then I guess we have to generate the anchors based on the original markdown headings and not the rendered HTML versions.

@Flet
Copy link
Contributor

Flet commented Jan 29, 2016

Indeed, just for clarification, github-slugger was created specifically for marky-markdown :)

I made it a package in case others wanted the functionality, and @wooorm was one of them!

For good measure, I've just added @ashleygwilliams and @chrisdickinson as collaborators and npm publishers. It's been great seeing the improvements to marky-markdown in the past few months, and I don't want to be the bottleneck for future progress! 👍

@revin
Copy link
Collaborator

revin commented Jan 30, 2016

OK, thanks for the history lesson and the detail, @wooorm and @Flet. I'm pretty new to marky, so everything earlier than the last month or two is just a black box to me 😄

Turns out to have been way more work than I was expecting, but I believe I have a more technically sound PR candidate now, thanks to all of your input. The commit message describes the changes in some detail.

@ashleygwilliams
Copy link
Contributor

closed by #133

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants