improving headline slugging to be more GH compliant. #4
Conversation
indexzero
left a comment
indexzero
left a comment
There was a problem hiding this comment.
Great start – added some additional test cases that I've seen in the wild.
|
|
||
| # a `code block` in the header | ||
|
|
||
|
|
There was a problem hiding this comment.
Other important test cases:
# Other punctuation, such as d.o.t.s, commas
# Emdash –– and dash --
# Ampersand &
# Backslashes// or slashes\\
# Complex code blocks like `/foo/bar/:bazz?buzz=foo`
# Bold formatting **like this**
# Italic formatting _like this_
# All !@# the $%^ colors &*( of ){} the |~ punctuation < "' rainbow +=
# Seriously all of them ... Alt + [q-|] œ∑´®†¥¨ˆøπ“‘«Might make more sense to have a more structured file (like a CSV of two columns output,input) e.g.
#test,# test
# 2 starts with number,#2-starts-with-numberThen you could generate the real .md file from that CSV as part of npm pretest or npm posttest (since we need it to verify what Github actually does since this algorithm is not part of the official GFM specification)
|
FYI did more digging on GFM itself:
|
clinejm
changed the title
dusave
left a comment
dusave
left a comment
There was a problem hiding this comment.
Can you export this from the package so we can leverage the slugifier in our own renderers?
| if (this.slugs[slug]) { | ||
| uniqueSlug = `${slug}-${this.slugs[slug]}`; | ||
| } | ||
| this.slugs[slug] += 1; |
There was a problem hiding this comment.
Could we save this.slugs[slug] in a local const? I think it would allow it to read cleaner.
const numOccurances = this.slugs[slug] || 0;
if (numOccurances) {
uniqueSlug = `${slug}-${numOccurances}`;
}
this.slugs[slug] += 1;There was a problem hiding this comment.
And maybe change slugs to slugCounts?
package.json
Outdated
| "browser": "lib/index.js", | ||
| "module": "src/index.js", | ||
| "main": "./index.js", | ||
| "browser": "./index.js", |
There was a problem hiding this comment.
Shouldn't main and browser still be lib/index.js?
There was a problem hiding this comment.
Yes. index.js should probably be src/index.js
test/index.test.js
Outdated
| assume(tree.find('#codething-in-the-header-morecode-txt')).to.have.length(1); | ||
| assume(tree.find('#codething-in-the-header-morecode-txt') | ||
| .find('a').prop('href')).is.equal('#codething-in-the-header-morecode-txt'); | ||
| assume(tree.find('#codething--in-the-header-morecode--txt')).to.have.length(1); |
There was a problem hiding this comment.
why are we assuming to have --? That seems like a bug to me.
There was a problem hiding this comment.
IMHO, if we run into a double space, or a character that can't resolve, I would prefer to reduce the - to a single for each, rather than multiple.
There was a problem hiding this comment.
I'll double check what GH is doing to ensure we match
* [refactor dist] Move `src/index.js` to `src/component.js` * [refactor dist] Move index.js to src/index.js * [fix dist] Update main, browser, & module.
src/index.js
Outdated
| import GithubSlugify from './gh-slugify'; | ||
| import URL from 'url-parse'; | ||
| import ReactMarkdownGithub from 'src/component.js'; | ||
| import GithubSlugify from 'src/gh-slugify.js'; |
There was a problem hiding this comment.
Shouldn't these be ./component and ./gh-slugify?
No description provided.