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

feat: set medium colour to yellow #306

Merged
merged 1 commit into from
Feb 23, 2019
Merged

feat: set medium colour to yellow #306

merged 1 commit into from
Feb 23, 2019

Conversation

whitneyit
Copy link
Contributor

As discussed in gotwarlost/istanbul#542, this
PR brings back the yellow colour for medium.

There are many good points raised in that PR, and this PR, aims to
replicate address those concerns for this repo.

As discussed in gotwarlost/istanbul#542, this
PR brings back the yellow colour for medium.

There are many good points raised in that PR, and this PR, aims to
replicate address those concerns for this repo.
.medium .chart { border:1px solid #666; }
.medium .cover-fill { background: #666; }
/* dark yellow (gold) */
.status-line.medium, .medium .cover-fill { background: #f9cd0b; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to me like a 'bright yellow' rather than dark yellow. How would you feel about #f1c50b instead (for .medium .chart as well)?

@whitneyit
Copy link
Contributor Author

I'm fine with any yellow colour. The colours I pulled were from the origin PR.

https://github.com/gotwarlost/istanbul/pull/597/files

Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️ I too am fine with any shade. My only ask would be to install https://colororacle.org/ and test it under different conditions.

@coreyfarrell
Copy link
Member

I just tried this out (nice utility btw). I think this PR is an improvement for deuteranopia (listed as common). Deuteranopia seems to make the current background red (for low coverage) appear as grey (the same as middle coverage). Unfortunately green (high coverage) appears yellowish, but it appears as a different yellow than what we use for middle coverage.

New color scheme:
image

With deuteranopia:
image

Original with deuteranopia:
image

So I think this is an improvement to switch medium coverage away from grey. I think it's less harmful for medium coverage to nearly look like high coverage than it is for low coverage to look exactly like medium coverage.

@JaKXz
Copy link
Member

JaKXz commented Feb 23, 2019

Thanks @coreyfarrell! I am good to merge if you are

@coreyfarrell coreyfarrell merged commit ed40be7 into istanbuljs:master Feb 23, 2019
@coreyfarrell
Copy link
Member

Thanks for the contribution @whitneyit!

@whitneyit
Copy link
Contributor Author

This was good news to wake up to! Cheers all

Yukaii pushed a commit to hackmdio/istanbuljs that referenced this pull request Mar 12, 2019
As discussed in gotwarlost/istanbul#542, this
PR brings back the yellow colour for medium.

There are many good points raised in that PR, and this PR, aims to
replicate address those concerns for this repo.
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