search

11ty / eleventy-plugin-syntaxhighlight

A pack of Eleventy plugins for syntax highlighting in Markdown, Liquid, and Nunjucks templates.
https://www.11ty.dev/docs/plugins/syntaxhighlight/
MIT License
129 stars 32 forks source link

Consider using semantic HTML elements instead of <div/span> with classes #6

Closed mathiasbynens closed 6 years ago

mathiasbynens commented 6 years ago

Instead of using <div>s (or <span>s) for marking parts of the code as highlighted/added/removed, why not just use semantic HTML elements?

.highlight-line-active → <mark> .highlight-line-remove → <del> .highlight-line-add → <ins>

Would you be open to a PR that makes this change?

zachleat commented 6 years ago

Hmm, yes I like this—but can the PR use both methods for now? And then I’ll make an issue to weigh whether or not to remove the classes in a major version change. Don’t want to break backwards compat there

mathiasbynens commented 6 years ago

Hmm, replacing <div> with e.g. <mark> could still be considered a “breaking” change if styles implicitly rely on the element being display: block.

Either way, I’ll kick off a PR and we can take the discussion there.

zachleat commented 6 years ago

Looks like they were just <span>’s so we’re safe I think.

Fixed by #7

zachleat commented 6 years ago

Thinking a little bit more about this, I think it’s minimal risk here since we have provided both highlight-line and highlight-line-${type} classes for each of these and there was no reason to tie styles to the span elements with those classes available. If you think we should do a major version bump for this instead of v1.0.6 we could do that to. I don’t think major bumping this one is going to be a huge deal.

mathiasbynens commented 6 years ago

They became <span>s with https://github.com/11ty/eleventy-plugin-syntaxhighlight/pull/5.

I don’t have a strong opinion. Version numbers are free, so when in doubt, it’s fine to use major bumps IMHO.