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

Fix "missed branch" background color #98

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cboos
Copy link

@cboos cboos commented Apr 13, 2020

First, thanks for the new release, with the new branch coverage support

I noticed that the choice of background color can sometimes be problematic, due to the syntax highlighting of the code. The comments are not supposed to be executed but the syntax highlighting is not perfect and sometimes code is rendered as comment when it's not. If you have a .missed-branch style on that, this becomes unreadable:

image

So I propose this one instead:

image

At the occasion of tweaking the CSS, I noticed that only the .missed-branch:nth-child(odd) was ever used... which lead to a follow-up change, moving the status class from the <li> to its parent <div>.

The syntax highlighting can sometimes misrepresent code as comment,
and when that's the case, the light gray can't be read against
a dark tan background.
@cboos cboos changed the title Fix branch missing background color Fix "missed branch" background color Apr 13, 2020
@PragTob PragTob changed the base branch from master to main July 5, 2020 10:33
@PragTob
Copy link
Collaborator

PragTob commented Aug 21, 2020

👋

Heyo, thanks for your contribution 💚 and sorry for not giving it attention earlier :(

I'm a bit confused why the class was moved to the div from the li? 🤔

Also color choice wise (yayyy bike shedding! ;P ) I'm not sure this is the best... I'll ponder it a bit. It's a change for people but you're absolutely right, readability of what's missed should trump color preferences. Another question would be if we could update the library we're using for the highlighting to detect these cases but well ruby is notoriously hard to highlight well.

Cheerio,
Tobi

@cboos
Copy link
Author

cboos commented Aug 28, 2020

I'm a bit confused why the class was moved to the div from the li? 🤔

Well, as I said, this was to get the existing CSS .missed-branch:nth-child(odd)/even working... The <div> elements are the direct "children" of the <ol> element, the <li> are always single child of their <div> hence .missed-branch:nth-child(even) will never apply. Now having said that, I wonder if this <ol><div><li> scheme is even legit HTML... so you probably might want to get rid of the <div> altogether.

Also color choice wise (yayyy bike shedding! ;P ) I'm not sure this is the best... I'll ponder it a bit. It's a change for people but you're absolutely right, readability of what's missed should trump color preferences. Another question would be if we could update the library we're using for the highlighting to detect these cases but well ruby is notoriously hard to highlight well.

You could provide an escape hatch for polyglots ;-) Have a configuration tweak or something that would allow for calling Pygments. Oh, there's even already a gem for doing that in a smart way, pygments.rb.

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