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

Convert leading tabs to spaces in fenced code blocks #127

Merged
merged 1 commit into from
Feb 8, 2016
Merged

Convert leading tabs to spaces in fenced code blocks #127

merged 1 commit into from
Feb 8, 2016

Conversation

revin
Copy link
Collaborator

@revin revin commented Jan 22, 2016

In #126 we have, among other things, a couple of rendering issues caused by the fact that markdown-it (probably correctly) doesn't do anything with tab characters in fenced code blocks; and browsers render them with the traditional eight character width, and sometimes visually collapse sequential tabs into one. Neither of these behaviors matches what GitHub does, which appears to be four-space substitution per leading tab.

I couldn't find anything in the markdown-it plugin ecosystem that already covered this, so I built a markdown-it plugin called markdown-it-expand-tabs that we can use to help fix these rendering glitches. It's on NPM now, so this PR pulls it in and updates the basic.md test fixture to include some tab-indented code we can translate and inspect.

@revin
Copy link
Collaborator Author

revin commented Jan 22, 2016

Also, I'm passing the plugin a hard-coded tab width of 4 spaces because that's how Github renders. However, it wouldn't be particularly difficult to also allow this to be overridden in marky's options parameter if we wanted to do that (and if so, please suggest a good name 😀).

@revin
Copy link
Collaborator Author

revin commented Jan 22, 2016

Looks like the plugin might need some work; it's bombing out of Travis on older Node versions.

@revin revin changed the title Convert leading tabs to spaces in fenced code blocks WIP: Convert leading tabs to spaces in fenced code blocks Jan 23, 2016
@revin
Copy link
Collaborator Author

revin commented Jan 23, 2016

All right, Travis went green; I'll squash and re-push

@revin revin changed the title WIP: Convert leading tabs to spaces in fenced code blocks Convert leading tabs to spaces in fenced code blocks Jan 23, 2016
@revin
Copy link
Collaborator Author

revin commented Jan 23, 2016

npm/newww#1678 suggests using a package's .editorconfig for tab width. That might be a cool addition to this, or maybe something for soon after.

@revin revin mentioned this pull request Jan 25, 2016
@@ -45,7 +46,7 @@ module.exports = function (html, options) {
}
}

var parser = MD(mdOptions).use(lazyHeaders).use(emoji, {shortcuts: {}})
var parser = MD(mdOptions).use(lazyHeaders).use(emoji, {shortcuts: {}}).use(expandTabs, {tabWidth: 4})
Copy link
Contributor

Choose a reason for hiding this comment

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

am i about to argue about tab spacing?
i'm about to argue about tab spacing.

thoughts on 2 vs 4? i'm a 2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

2 seems to be more Node-y, but from what I'd seen on github, they go with 4 on their markdown rendering, so I chose 4 here. I'm happy to go with whatever you like. "GitHub parity" doesn't necessarily have to mean 100% identical whitespace 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

hahahha no no, you (and GH parity) win. 4 it is!

@ashleygwilliams ashleygwilliams self-assigned this Jan 26, 2016
@ashleygwilliams
Copy link
Contributor

is this still WIP @revin? i fear i've lost track of it. also-- this is a breaking change ya? tentatively setting at 7.0.0

@ashleygwilliams ashleygwilliams added this to the 7.0.0 milestone Feb 8, 2016
@revin
Copy link
Collaborator Author

revin commented Feb 8, 2016

@ashleygwilliams no longer WIP, this should be good to go for now. I think this is technically a breaking change, but the main thing that would probably break is people's CSS for tab widths (unless someone somewhere is attempting to parse the rendered code blocks and is sensitive to the whitespace format), so it's pretty benign. 7.0 sounds like a reasonable milestone to me though.

ashleygwilliams added a commit that referenced this pull request Feb 8, 2016
Convert leading tabs to spaces in fenced code blocks
@ashleygwilliams ashleygwilliams merged commit 9be7a2a into npm:master Feb 8, 2016
@revin revin deleted the markdown-it-expand-tabs branch February 8, 2016 21:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants