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

Improve line numbers #149

Closed
Arcovion opened this issue Jun 5, 2014 · 10 comments
Closed

Improve line numbers #149

Arcovion opened this issue Jun 5, 2014 · 10 comments
Labels
stale-issue There has been no activity for a year.

Comments

@Arcovion
Copy link
Contributor

Arcovion commented Jun 5, 2014

I made a gist showing a few different ways to implement line numbers, let me walk you through the pros and cons...

Current:

  • Gutter can adapt to the width of the line count
  • Messy, introduces three new tag types to the HTML
  • Uses a mix of inline styles and classes (why not inline these styles? Avoid table styles unless needed #142)
  • Line numbers are copied to the clipboard when selecting and copying code
  • Not semantic

CSS only:

  • Doesn't need a named class, code is semantically wrapped and therefore easy to style
  • All styling is kept in the CSS (as it should be), the HTML needs no special treatment
  • When selecting and copying code the line numbers aren't added to your clipboard
  • Only adds line numbers up to 250
  • Semantically correct

Separate lines:

  • All the advantages of the previous method
  • Less CSS and no line limit thanks to CSS3
  • Gist and other sites like pastebin also wrap each line in a tag

Table with separate lines:

  • Line numbers are still copied to clipboard
  • Very robust and quite pretty, this HTML can be indented without affecting the code
  • Straightforward and simple, each line of the HTML has it's own number which in turn corresponds to a line in the code
  • Just as bad semantically as it is now, could be better but I can't bare to wrap more tags around it

Just throwing ideas out here, I think all three methods have advantages over the current system - I don't like the markup and CSS being used atm.

I propose adding the last two options, I don't know how difficult it is to wrap each line in a tag, but there is definitely a reason why it's the most common approach.

With this, people can do their zebra striping and have fluid gutters, but are also are given the option to use the CSS3 counter instead of HTML and copy code without the line numbers etc. Doing this with only CSS wouldn't need line numbers enabled at all, but that approach is basically a hack.

@jneen
Copy link
Member

jneen commented Jun 5, 2014

Those styles aren't inlined because it's reasonable that someone might want to override them, which they can't do if it's in the style attribute. The line numbers don't get copied into the clipboard with the current approach - that was explicitly why I chose it. Do they for you? In which browser?

@Arcovion
Copy link
Contributor Author

Arcovion commented Jun 5, 2014

I'd prefer no inline styles at all, like my examples.
All browsers, can you provide an example that doesn't? I guess they aren't interspersed with each line so they're easy to remove, that's something I didn't consider.

@jneen
Copy link
Member

jneen commented Jun 5, 2014

Run bundle exec rackup and go to localhost:9292/ruby?line_numbers=1. I just did this, and selected the code and pasted it into an editor with no line numbers.

@Arcovion
Copy link
Contributor Author

Arcovion commented Jun 5, 2014

I just get

EncodingError at /ruby
Bad encoding: CP850,IBM850,locale,external. Please convert your string to UTF-8.

might be a Windows thing.

@Arcovion
Copy link
Contributor Author

Arcovion commented Jun 5, 2014

Regardless, I think using CSS line numbers is much better in principle as it separates the content from the style. I see why you chose the current table style now though and why it seemed so odd :P

@jneen
Copy link
Member

jneen commented Jun 5, 2014

Would you mind spending some time hunting down that bug? I think it's important that you're able to look at the visual specs.

@Arcovion
Copy link
Contributor Author

Arcovion commented Jun 5, 2014

Encoding to utf-8 here loads the main page:

<%= Rouge.highlight(sample.demo.encode('utf-8'), sample, @formatter) %>

at rouge/spec/visual/templates/index.erb:10, and encoding @sample at app.rb:51

No dice with /ruby though, I get:

Encoding::CompatibilityError at /ruby
incompatible character encodings: UTF-8 and CP850

However, with the ones I can view, I see what you mean now; I can choose to select the line numbers or not, works nicely. It's not as nice as the CSS method, but definitely better than my table version.

What do you think about adding the two CSS methods then?

@jneen
Copy link
Member

jneen commented Jun 5, 2014

Ah, so it's being read as the wrong encoding. Do you think you could add encoding: 'utf-8' to any calls to File.read that you find? There's one in lib/rouge/lexer.rb for the demos, and one in spec/visual/app.rb - both of those could use a # coding: utf-8 comment as well.

Thanks!

@Arcovion
Copy link
Contributor Author

Arcovion commented Jun 6, 2014

Yep, just adding .encode('utf-8') to the end of those lines works.
Edit: Didn't try encoding: 'utf-8' as a parameter or using # encoding: utf-8, but I assume they'd work too.
Edit2: Opened a separate issue regarding encoding.

@stale
Copy link

stale bot commented Jun 19, 2019

This issue has been automatically marked as stale because it has not had any activity for more than a year. It will be closed if no additional activity occurs within the next 14 days.

If you would like this issue to remain open, please reply and let us know if the issue is still reproducible.

@stale stale bot added the stale-issue There has been no activity for a year. label Jun 19, 2019
@stale stale bot closed this as completed Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale-issue There has been no activity for a year.
Projects
None yet
Development

No branches or pull requests

2 participants