sight icon indicating copy to clipboard operation
sight copied to clipboard

Add word wrapping option

Open tsenart opened this issue 8 years ago • 10 comments

Fixes #66 and supplants #88.

tsenart avatar Dec 31 '16 16:12 tsenart

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).

YellowAfterlife avatar Dec 31 '16 16:12 YellowAfterlife

Would you please post an image showing the difference?

tsenart avatar Dec 31 '16 17:12 tsenart

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.

YellowAfterlife avatar Jan 01 '17 00:01 YellowAfterlife

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

tsenart avatar Jan 01 '17 00:01 tsenart

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.

YellowAfterlife avatar Jan 01 '17 01:01 YellowAfterlife

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

tsenart avatar Jan 01 '17 16:01 tsenart

https://github.com/YellowAfterlife/sight/commit/8fa28b4c4612f8f2b3bd5d53d05a671836741404 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.

YellowAfterlife avatar Jan 01 '17 19:01 YellowAfterlife

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

tsenart avatar Jan 01 '17 19:01 tsenart

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?

YellowAfterlife avatar Jan 02 '17 08:01 YellowAfterlife

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

tsenart avatar Jan 02 '17 10:01 tsenart