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

update text and background color for contrast. add fill and border to… #1952

Closed
wants to merge 1 commit into from

Conversation

bowenli
Copy link
Contributor

@bowenli bowenli commented Oct 25, 2016

Fixes #1943

  • On contrast mode, text color set to black, background set to white (no gradient)
  • Label background set to white (no transparency)
  • Visual update of nodes

untitled-1

  • Note: there is an existing bug where sometimes the node rendering has a gap between the fill and border. This PR does not fix this.

@bowenli bowenli added bug Broken end user or developer functionality; not working as the developers intended it component/ui Predominantly a front-end issue; most/all of the work can be completed by a f/e developer labels Oct 27, 2016
@davkal
Copy link
Contributor

davkal commented Oct 28, 2016

Looking at the big screen, my first reaction is that it looks less slick, but that was expected. To qualify this a bit more, I feel like the number of things on the screen vying for my attention increased.

On closer inspection I feel like the 3-color version of a node looks a bit cluttered with the two outlines.

screen shot 2016-10-28 at 13 35 08

I've created a version with two colors that makes use of a third transparent outline.

screen shot 2016-10-28 at 13 34 39

Here is the diff https://gist.github.com/davkal/c290c6fea25c6930eb15b43effac94d7

@foot WDYT?

@rade
Copy link
Member

rade commented Oct 28, 2016

How is this supposed to work with metrics-on-canvas?

@davkal
Copy link
Contributor

davkal commented Oct 28, 2016

How is this supposed to work with metrics-on-canvas?

The background turns transparent again:

screen shot 2016-10-28 at 15 02 47

@davkal davkal assigned foot and unassigned davkal Oct 28, 2016
@foot
Copy link
Contributor

foot commented Nov 2, 2016

Nice variations guys!

  • Solid rank color on nodes does look a bit jellybeany and "less slick"
  • But! makes rank much much clearer which is cool.
  • But! Rank color is not actually that useful right now. [RFC] Separate node colors #1567

On 2 vs 3 tone

  • I like the two tones
  • But! I also think having a node as an "unbroken" solid, unclutters things and simplifies the diagram a bit.

@davkal
Copy link
Contributor

davkal commented Nov 2, 2016

Unbroken variant with 2 tones:

screen shot 2016-11-02 at 11 19 18

To add detail we could introduce the empty space at close zoom levels 👀

@foot
Copy link
Contributor

foot commented Nov 2, 2016

Yep! Either the 3-tone, or the transparent (or bg colored) ring could make an appearance at closer zoom levels to make it look more... slick?

@rade
Copy link
Member

rade commented Nov 2, 2016

  • On contrast mode, text color set to black, background set to white (no gradient)
  • Label background set to white (no transparency)
  • Visual update of nodes

I suggest we do the first two only, which seem uncontroversial.

@bowenli
Copy link
Contributor Author

bowenli commented Nov 2, 2016

The outer border ended up way bigger than I envisioned. The outline controls were a bit difficult to accurately position. Here is how I designed it:
screen shot 2016-11-02 at 14 23 46

@bowenli
Copy link
Contributor Author

bowenli commented Nov 2, 2016

I think the 2 tone version is still nicer than it currently is.

@pidster
Copy link
Contributor

pidster commented Nov 8, 2016

If the original issue is about readability, the PR fixing it should address just that and not a change of colour - which seems unrelated to me.

@davkal
Copy link
Contributor

davkal commented Nov 10, 2016

I'm fine with the first two items as well.

As for the node changes, I would propose to go with 2 tones, but have the light one be lighter than what getNodeColorLight currently returns for the normal mode, and maybe the current getNodeColorLight for the HC mode. Also, getNodeColorLight is probably used in other places too, so you may need two different one.

@rade
Copy link
Member

rade commented Nov 10, 2016

This has been dragging on for far too long. Move the node change into a separate PR, leaving just the first two items in this PR. Review. Merge.

@bowenli
Copy link
Contributor Author

bowenli commented Nov 10, 2016

Closing this PR.

Text only change here: #2006

@bowenli bowenli closed this Nov 10, 2016
@fbarl fbarl deleted the 1943-contrast-improvements branch January 13, 2020 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Broken end user or developer functionality; not working as the developers intended it component/ui Predominantly a front-end issue; most/all of the work can be completed by a f/e developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

High contrast mode needs more contrast
5 participants