Workday / mps-code-reviewer

Code Review for JetBrains MPS providing integration with Bitbucket
Apache License 2.0
15 stars 7 forks source link

Mock bitbucket connection via prefs #3

Closed ty1824 closed 6 years ago

justinhorton commented 6 years ago

The luminance calculation for altering the text color doesn't seem to give white in all cases that I expect white (e.g. for green). Can we just make it white across the board for these other activities?

or change it to:

// convert to grayscale 
final float grayVal = background.getRed() * 0.299f + background.getGreen() * 0.587f + background.getBlue() * 0.114f; 
return grayVal > 186 ? Color.BLACK : Color.WHITE;

The threshold value is semi-arbitrary, taken by suggestion from here: https://stackoverflow.com/a/946734

The color calculation is mentioned there and in https://www.w3.org/TR/AERT/#color-contrast

Also, did you intentionally change the colors from what's shown on Bitbucket? Seems like they're pretty close, but some aren't exactly right.

These are the Bitbucket colors:

OPENED("opened the pull request", new Color(74, 103, 133)) 
MERGED("merged the pull request", new Color(20, 137, 44)) // this is your specified color in integer RGB notation
REVIEWED("marked needs work", new Color(246, 146, 50)) 
APPROVED("marked approved", new Color(20, 137, 44))
UNAPPROVED("marked unapproved", new Color(208, 65, 55)) 
RESCOPED("updated the pull request", new Color(246, 195, 66))

(Merged is the same color as Approved in Bitbucket--did you change this color to help distinguish it? I think distinguishing shouldn't matter much, since it'll usually appear at the top of the activity feed and we'll later probably want to show the branch and merge commit hash.)

justinhorton commented 6 years ago

your colors: image

Bitbucket colors: image

Maybe the Bitbucket colors are a bit harsh when used as a larger background? I do think the green should use white text, though; otherwise, it just looks inconsistent.

justinhorton commented 6 years ago

I'd suggest a minor change to the MockBitbucketConnection as follows:

private int getMockPullRequestId() { 
  return getConfig().getMockPullRequestId(); 
} 

@Override 
protected MockBitbucketConfiguration getConfig() { 
  super.getConfig() as MockBitbucketConfiguration; 
}

In ReviewPlugin, you may want to pull out the connection initialization, since it's written identically twice. Everything else including the misc. fixes looks good.

ty1824 commented 6 years ago

Re: colors - yeah, I used the Mac color sampling tool to get the colors... which was silly. The one color that was different was when I inspected the element and copied the string.

As far as the contrast, I was going off of https://stackoverflow.com/questions/3942878/

I didn't like the fact that green led to black either, I could have messed up the implementation.