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

resolves #264 add line highlighting option #1426

Merged

Conversation

mojavelinux
Copy link
Contributor

  • add line highlighting option to HTMLLinewise formatter
  • add dedicated formatter to add line highlighting (only marks highlighted lines)

@pyrmont pyrmont self-assigned this Feb 5, 2020
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Feb 5, 2020
@pyrmont
Copy link
Contributor

pyrmont commented Apr 3, 2020

Sorry about the delay, @mojavelinux. I got a bit side-tracked with various things.

@jneen has expressed in the past a reluctance to add further formatters to Rouge. @jneen, what do you think about this PR?

@pyrmont pyrmont added maintainer-action The PR has been reviewed but action by a maintainer is required and removed needs-review The PR needs to be reviewed labels Apr 3, 2020
@mojavelinux
Copy link
Contributor Author

Thanks. As I said in #264, "If you think the additional formatter is unnecessary, I can remove it."

@jneen
Copy link
Member

jneen commented Jul 21, 2020

This seems okay to me, mostly because it's its own class. My major reluctance is to adding options to existing formatters.

- add line highlighting option to HTMLLinewise formatter
- add dedicated formatter to add line highlighting (only marks highlighted lines)
@mojavelinux mojavelinux force-pushed the issue-264-html-line-highlighter branch from 0c153de to 8275950 Compare August 26, 2020 06:14
@mojavelinux
Copy link
Contributor Author

Thanks for reviewing. I've gone ahead and removed the change that introduced the additional options to the existing formatter. Let me know if you need me to make any other changes.

@nektro
Copy link

nektro commented Oct 2, 2020

Any updates on this / is there anything blocking?

@jneen
Copy link
Member

jneen commented Oct 2, 2020

Looks good to me. It's using a \n for a newline, which seems to indicate it needs to be in a <pre> block of some kind, rather than using divs or otherwise. But I think it's okay. There's so many ways to format in HTML that I consider the builtin HTML formatters here to be more like... documentation. Or just the very basic use cases. I'm +1 to merge.

@jneen jneen merged commit d7b60c0 into rouge-ruby:master Oct 2, 2020
@jneen
Copy link
Member

jneen commented Oct 2, 2020

and merged! thanks all <3

mattt pushed a commit to NSHipster/rouge that referenced this pull request May 19, 2021
- add line highlighting option to HTMLLinewise formatter
- add dedicated formatter to add line highlighting (only marks highlighted lines)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer-action The PR has been reviewed but action by a maintainer is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants