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

JENKINS-8958 Code coverage color scheme not suited for color blind users #95

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@cizezsy
Copy link
Contributor

commented Mar 8, 2018

Add global config which can enable colorblind mode.

I use a Chrome extension which simulates the website as a color vision impaired person would see. And the following is the results.
Normal see it:
normal

Red-blind see it:
red-blind
Green-blind see it:
green-blind
Blue-blind see it:
blue-blind

@@ -0,0 +1,123 @@
table.source {

This comment has been minimized.

Copy link
@jeffpearce

jeffpearce Mar 10, 2018

Contributor

My concern with duplicating the entire css file is that anyone wanting to change the layout would have to change in two places (and might miss this one). Looking at the layout control https://github.com/jenkinsci/jenkins/blob/master/core/src/main/resources/lib/layout/layout.jelly#L46-L49 I see the css parameter is deprecated.

This suggests a slightly different approach, which would be to move the css path into a <script> tag inside of an <l:style> control. Then we could provide a color-blind css file that has just the overrides (presumably the colors). For regular mode, just import the base stylesheet, and for color-blind mode import the base stylesheet, then import the color-blind stylesheet. This has the advantage that we could easily provide more than one color-blind mode in the future should we want to.

This comment has been minimized.

Copy link
@cizezsy

cizezsy Mar 11, 2018

Author Contributor

Oh, You are right. Thanks for your advice :)

@@ -47,6 +41,9 @@
@ExportedBean(defaultVisibility = 2)
public class CoverageResult implements Serializable, Chartable {

private static final String DEFAULT_CSS_FILE = "style.css";
private static final String COLOR_BLIND_CSS_FILE = "style-color-blind.css";

This comment has been minimized.

Copy link
@jeffpearce

jeffpearce Mar 14, 2018

Contributor

I don't think these two constants are being used anymore

@oleg-nenashev
Copy link
Member

left a comment

Just minor comments

*
* @return whether enable color blind mode in global config
*/
public boolean isEnableColorBlindMode() {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Nice2 have: @since TODO for new APIs

import hudson.plugins.cobertura.CoverageChart;
import hudson.plugins.cobertura.Ratio;
import hudson.model.*;
import hudson.plugins.cobertura.*;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

It is generally preferable to avoid it in the production code, because there is a risk of ambiguity if one of the packages introduces a competing class.

/**
* load global config from disk
*/
public DescriptorImpl(Class<? extends Publisher> clazz) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Is it actually used anywhere?

This comment has been minimized.

Copy link
@cizezsy

cizezsy Mar 15, 2018

Author Contributor

The comment of Descriptor said Descriptor can persist data just by storing them in fields. However, it is the responsibility of the derived type to properly invoke save() and load().
So I call save() in configure() to persist global configuration. And the comment of load() said The constructor of the derived class must call this method. (If we do that in the base class, the derived class won't get a chance to set default values.). So I follow the comment to implemented its two constructors and call load method.

This comment has been minimized.

Copy link
@jeffpearce

jeffpearce Mar 15, 2018

Contributor

It provides a config setting on the Jenkins global settings page:

screenshot 2018-03-15 07 36 24

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

@jeffpearce

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2018

That was my initial thought as well. I could see doing the work in Cobertura now, and converting it to use the global setting later. Do you think that's worthwhile, or should we close this one?

@jeffpearce

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2018

I guess the global config might have specific colors though, so it's not clear it would be a 1:1 translation to this code

@steven-foster

This comment has been minimized.

Copy link

commented Mar 17, 2018

Maybe a User Property instead of global? or both

@jeffpearce

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2018

@steven-foster can you elaborate? I'm not sure what the difference is between User and global property

@cizezsy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

@jeffpearce Should I extended color-blind mode more than just in coverage bar but also in trend image and so on?

@jeffpearce

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2018

That makes sense to me

@cizezsy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

OK, the trend image and source line mark are also unsuitable for color vision impaired person. I think I should change them all.

@jeffpearce

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2018

👍 Thanks again for the hard work on this plugin

@cizezsy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2018

@jeffpearce I add more diversity to the origin chart lines. So not just for the color-blind people but for normal people can differentiate it easier.

Origin:
origin

Now
Normal see it:
normal
Red-blind see it:
red-blind
Green-blind see it:
green-blind
Blue-blind see it:
blue-blind

what do you think of it?

cizezsy added some commits Mar 17, 2018

@steven-foster

This comment has been minimized.

Copy link

commented Mar 17, 2018

@jeffpearce a UserProperty would allow each user to configure it for themselves, rather than a global configuration which would change it for everybody using the Jenkins instance. It's just a suggestion, there might be a reason not to do it this way :)

@jeffpearce

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

@steven-foster - that would be good. I could see this as a next step, and UserProperty down the road, so it could be configured per-user

@jeffpearce

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

@cizezsy - I'm sorry, this fell off my radar. Can you remind me where we're at? Is this ready for another round of review?

@cizezsy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

@jeffpearce My fault, I do not say it clearly. I have changed the line shape of trend image so that it will be more friendly to the color-blind user. But, the original image is not clear also for normal users. So I think we should not add this change just for colorblind mode but for all users. Maybe I should start another pull request like "improve trend image" to do this thing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.