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

Add word wrapping option #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add word wrapping option #89

wants to merge 1 commit into from

Conversation

tsenart
Copy link
Owner

@tsenart tsenart commented Dec 31, 2016

Fixes #66 and supplants #88.

@YellowAfterlife
Copy link

This adds a wrap option but does not modify layout (line number column) to not break line number display on wrap (see sample from original issue).

@tsenart
Copy link
Owner Author

tsenart commented Dec 31, 2016

Would you please post an image showing the difference?

@YellowAfterlife
Copy link

YellowAfterlife commented Jan 1, 2017

With my PR:
image
With this PR:
image
Most of the odd-ish code in my PR is to switch the highlighter to generate tables instead to workaround this issue.

@tsenart
Copy link
Owner Author

tsenart commented Jan 1, 2017

OK, is there a pure CSS way to achieve this?

@YellowAfterlife
Copy link

As far as I understand, it would not be possible without changing something in the existing structure, since currently the lines are not even explicitly wrapped in any particular element, meaning that you cannot measure their height or apply selectors to them.

The simplest solution would probably be this, which has each line wrapped in a <span> (instead of having a blank <span> before the line contents) to be able to give it padding-left and stick a ::before pseudoelement into that area. Note that this would not automatically resize all line counter elements to fit required width, however, so additional rules might be required to make the line number area wider if there are more than 100, 1000, etc. lines in the output.

@tsenart
Copy link
Owner Author

tsenart commented Jan 1, 2017

@YellowAfterlife: I like that direction a lot more. Would be happy to review a patch on top of this PR to fix the issue.

@YellowAfterlife
Copy link

YellowAfterlife commented Jan 1, 2017

YAL-Forks@8fa28b4
I don't truly like the line-digits-# part but that's the best it gets without giving every line-node a width property explicitly or rewriting everything to use tables.
Had only come to realization that the whole thing with line-numbers is a hack on top of hljs and there was no use in my previous attempt of implementing them without modifying hljs.js.

@tsenart
Copy link
Owner Author

tsenart commented Jan 1, 2017

I don't really want to maintain a fork of the HLJS library so the solution needs to build on top of its quirks.

@YellowAfterlife
Copy link

Well, you already do, for two years now. Would you want a PR that updates hljs to current version and adds tweaks for line numbers (probably by making a wrapper .js for both background.js and options.js to use) without modifying hljs then?

@tsenart
Copy link
Owner Author

tsenart commented Jan 2, 2017

@YellowAfterlife: Oh, wow. Memory runs short indeed. Thanks for spotting that. So, yes I'd very much appreciate that contribution. Thank you!

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

Successfully merging this pull request may close these issues.

2 participants