Fix Solarized line number colors#1477
Conversation
…zed-line-number-colors
Anteru
left a comment
There was a problem hiding this comment.
I have only one very minor comment -- otherwise this is really great work, and I do appreciate the time you put into testing this properly.
|
@Anteru are there any other fixes you would like me to include? |
|
Nope, looks good, I'll merge as soon as I have some more time :) |
|
|
||
| @property | ||
| def _pre_style(self): | ||
| return 'line-height: 125%; margin: 0;' |
There was a problem hiding this comment.
You're seeing (I guess) because your theme doesn't have margin/padding on the paragraph either. I'd try to override the style if you care about specific padding/margin. That said, the margin: 0 was not intentional (I think) so I'll revert that for 2.7.3.
There was a problem hiding this comment.
Hi! I'm sorry if I'm a bit too late, but I'm pretty I've introduced the margin: 0 rule for a reason. Since I've worked on this one some time ago, I'm not sure what exactly was it, but I'd guess it had something to do with making the table and non-table styles look more similar. An alternative might be that it had something to do with highlighting lines/special lines. There was a lot of different combinations of styles and highlight rules etc. and I believe margin: 0 was introduced to make everything work better in regard to some of these combinations.
Having said that, if it actually looks better without margin: 0 and it doesn't break anything else, I'm totally OK with removing this rule :)
There was a problem hiding this comment.
I can't find the case where it would matter, and it does break various themes -- if you can point me to a problematic case then we can revisit alternative solutions. Ideally theme authors should have a way to override Pygments CSS rules, but for now this seems to be causing too much fallout :(
There was a problem hiding this comment.
I tried to add all problematic cases to the test suite :P But while working on this a lot of dependencies came up and it's possible I've changed one thing too many ;) If you could just add a proper test with an explanation/justification, I would be very grateful. Right now, unfortunately, I'm not able to spend any time fixing this or checking why exactly was this change made. Reverting it seems like a good solution, as long as it's documented ;)
There was a problem hiding this comment.
Fair enough. I ran a bunch of manual tests and also spot-checked the test cases, and couldn't find a problematic case. Was hoping you'd remember something particularly problematic, but given the feedback we got, it seems that any rule changing the margin will cause more harm than good :(
|
@Anteru, please allow me a quick question here: when is expected release date of 2.7.3? |
|
Today. |
|
Thanks! |



Fixes #1356.
Changes:
get_style_defs(and removed fromCSSFILE_TEMPLATE)noclassesoption was updatedStyleclass received new attributes for: