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

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

Closed
mathiasbynens opened this issue Sep 23, 2018 · 5 comments
Milestone

Comments

@mathiasbynens
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

Fixed by #7

@zachleat zachleat added this to the v1.0.6 milestone Sep 25, 2018
@zachleat
Copy link
Member

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
Copy link
Contributor Author

They became <span>s with #5.

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

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

No branches or pull requests

2 participants