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 CSS class to pre tag instead of code tag #191

Merged
merged 1 commit into from
Nov 17, 2014

Conversation

eugeneius
Copy link
Contributor

Background colours have been broken since v1.4.0, as noted here: #141 (comment)

This is because the highlight CSS class is added to the code tag, which is an inline element. Adding the class to the surrounding pre tag makes the background display correctly, since it's a block element.

Background colours have been broken since `v1.4.0`, as noted here:
rouge-ruby#141 (comment)

This is because the highlight CSS class is added to the `code` tag,
which is an inline element. Adding the class to the surrounding `pre`
tag makes the background display correctly, since it's a block element.
@Arcovion
Copy link
Contributor

In that thread I mentioned wanting display: block on tags:

Edit: Actually just keep the background-color in the block via display: block - it won't make any difference as long as the pre is set to the same colour anyway. This seems to be how other syntax highlighters handle it.

The spec suggests that the most semantic way is to use a tag;

 doesn't always mean code, that's why it was changed before.

So Rouge just applies colour to the tags everywhere without any block styling, therefore I add it myself via pre code { display: block } which wasn't needed before when it used

. Other highlighters simply add display: block to whatever selector you are using - pre code, .highlight etc. along with the background-color.

I can't see the harm in just adding display: block to the generated CSS, it's just a more semantic way to fix this problem IMO, what do you think?


Off topic:
There's a couple of other issues on backgrounds, one regarding inline behaving differently and adding backgrounds where they shouldn't be (#155), and one where I was going to fix the visual styles and discussed this stuff (#154), but the visual styles were just magically fixed one day and I don't know how... I assume the background was just added to another tag.

If you try this:

rougify style thankful_eyes > syntax1.css
rougify style monokai > syntax2.css

One has a background colour, one doesn't, most of them don't have one IIRC.

What should be the default? Should users be expected to add their own colour to the

, or should they all have backgrounds and there be an option to disable them? Right now it feels inconsistent, I suggested an option like :background => true in #154 or maybe it should just stay like it is now?

That's one more breaking change to consider for the next version of Rouge if this changes too.

@jneen
Copy link
Member

jneen commented Nov 16, 2014

This feature has seen really a lot of churn lately, and I'd like to solve it once and for all, especially since changes like these can break user code. I don't care nearly as much about html semantics as I do about not breaking user code, and also not nearly as much as I care about compatibility with existing rendered pygments stylesheets.

So if we aren't getting the background color from a given pygments stylesheet, I consider that a high-priority bug.

@Arcovion
Copy link
Contributor

I haven't used pygments, so I found this monokai style and it has no display: block so you'd imagine they use

 instead of . Looking at their official website demos confirms this, they actually use div.syntax > pre.
With highlight.js which I used to use, you get pre > code.hljs and they add display: block to the hljs class which is what I'm suggesting.

I think display: block should be added to the CSS in any case, Rouge only outputs block level stuff with a

 or 
tag which means it's backward compatible to make everything it styles a block.

People who aren't using the generator to make their CSS and are using some other pygments stylesheet would need to add it to their CSS themselves, but it's so trivial to do and could be noted in the changelog. This PR is the right way to fix compatibility for those people without changing the stylesheet though, and it keeps the tag in too so it's still an improvement over previous releases. If you do merge this, consider adding an option to keep current HTML output though...

@jneen
Copy link
Member

jneen commented Nov 17, 2014

I think what I'm going to do is merge this for this version (since it's what people's stylesheets expect), and re-think the assumptions of the Theme generator and the HTML formatter for 2.0. I'll start an open thread to discuss that track.

jneen added a commit that referenced this pull request Nov 17, 2014
Add CSS class to pre tag instead of code tag
@jneen jneen merged commit 50571e7 into rouge-ruby:master Nov 17, 2014
denisdefreyne added a commit to nanoc/nanoc that referenced this pull request Nov 30, 2014
Rouge’s HTML formatter was changed so that the class appears on the
`pre` element rather than the `code` element.

See also rouge-ruby/rouge#191.
cvkef added a commit to cvkef/slate that referenced this pull request Mar 7, 2015
The bug introduced because of the rouge gem updated to version 1.8.0
[d7d2701] and it is rooted back in version 1.7.4 with the merge of
rouge-ruby/rouge#191.
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.

3 participants