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

2951 contrasting pie and bar inside text #3130

Merged
merged 17 commits into from
Oct 25, 2018

Conversation

rmoestl
Copy link
Contributor

@rmoestl rmoestl commented Oct 19, 2018

Improves a few things around text font styling for pie and bar. Specified and discussed in #2951.

Rough overview:

  • If no custom font color is specified in trace, an inside text label will render with a color contrasting the bar / pie fill color. This improves out-of-the-box readability.
  • Adds support for array (arrayOk) font properties in pie.
  • Bar textposition: 'outside' labels pushed inside by a bar being stacked on top of it are rendered with inside text font properties now (bugfix).
  • Font properties are now applied correctly throughout a selection interaction as well (bugfix).

- Bar text labels specified as `textposition: outside` being pushed
  `inside` (to avoid collisions with bars stacked on top) were not
  styled as inside labels.
- Parallel work being pie titles and 3d tick adjustments.
- Note: even before the introduction of defaulting to
  contrasting colors for inside text labels, font
  inheritance wasn't working throughout selection.
- Test ensures a bar's color opacity is taken into account when
  calculating a contrasting inside text color.
- Reason: both reading and writing assert statements was more
  cumbersome before because the conversion of colors into their
  rgb form had to be done in client code that called the font color
  assert functions.
@antoinerg
Copy link
Contributor

Very nice improvements for the legibility of the figures! 🎉

@rmoestl
Copy link
Contributor Author

rmoestl commented Oct 19, 2018

Very nice improvements for the legibility of the figures! tada

Thanks Antoine! 🙇‍♂️

@alexcjohnson
Copy link
Collaborator

in bar_attrs_relative why does text inside the orange bars end up white?

screen shot 2018-10-22 at 1 30 26 pm

Elsewhere (like hover labels, and pie labels elsewhere in this PR) that orange color keeps black text as a contrast.

screen shot 2018-10-22 at 1 28 30 pm

BTW very nice that the text that got pushed out of the small red bar turned to black even though the text inside the red bars is white 🎉

- Standard pie colors means no marker colors set explicitly.
- Reason: calculation of inside text color when bar has no
  explicit marker color set has been fixed in a previous commit.
@rmoestl
Copy link
Contributor Author

rmoestl commented Oct 23, 2018

in bar_attrs_relative why does text inside the orange bars end up white?

The problem was I had no test to verify that contrasting text colors are correct if no explicit marker colors are set. I mistakenly assumed calcdata[i].mc is always set just like it is the case for pie where calcdata[i].color has always a value. I've added a test to pie tough to cover the "no marker colors set" case.

@etpinard etpinard added bug something broken feature something new status: reviewable labels Oct 23, 2018
@alexcjohnson
Copy link
Collaborator

@rmoestl looks fantastic!

@etpinard I have one question about how this will play with RCE (cc @nicolaskruchten @VeraZab) - which currently doesn't seem to support (inside|outside)textfont at all but presumably at some point it will. I'm assuming because insidetextfont.color is undefined by default, we wouldn't show the corresponding color picker - so perhaps we should accept and fill in something like 'auto' instead? (see #3109 (comment) for discussion of null and other options)

@nicolaskruchten
Copy link
Contributor

We can certainly add widgets for these attributes to RCE.

In general, this kind of layered automatic computation is a bit tricky for a UI... we've resorted to this kind of thing in the figure-wide hover label spec:

image

@alexcjohnson
Copy link
Collaborator

OK great - then lets keep this as is -> 💃

I still have concerns about how stuff like this will interact with templates but those should be addressed in a broader context.

@rmoestl rmoestl merged commit d5ebb50 into master Oct 25, 2018
@rmoestl rmoestl deleted the 2951-contrasting-pie-and-bar-inside-text branch October 25, 2018 05:33
@etpinard etpinard added this to the v1.42.0 milestone Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants