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

fix barCharts rendering for zero values #41

Merged
merged 1 commit into from
Nov 25, 2020
Merged

fix barCharts rendering for zero values #41

merged 1 commit into from
Nov 25, 2020

Conversation

koorosh
Copy link
Contributor

@koorosh koorosh commented Nov 20, 2020

Resolves #40

This is a fix for regression that was caused when d3 package was
replaced by d3-scale package and others. Latest version of d3-scale
package introduced change related to the issue d3/d3-scale#117
that caused to half fill barChart for 0 value.

Now, normalizeClosedDomain checks if domain start and end values
are equal or not, and in case they are equal, it increases last value
by one. Note, it doesn't change actual values, but only domain range of
possible values.

Most of the time barChart renders bars with range from 0% to 100% and
it should show 0% value when we have values [0, 0].

  • With introduced "breaking" change in d3-scale, it returns middle point
    for range [0, 0] and it equals 50% (for range0 from % to 100%).
  • With fix and as it was before, for 0 value scaling within [0, 0] domain
    will return 0%

This is a fix for regression that was caused when d3 package was
replaced by d3-scale package and others. Latest version of `d3-scale`
package introduced change related to the issue d3/d3-scale#117
that caused to half fill barChart for 0 value.

Now, `normalizeClosedDomain` checks if domain start and end values
are equal or not, and in case they are equal, it increases last value
by one. Note, it doesn't change actual values, but only domain range of
possible values.

Most of the time barChart renders bars with range from 0% to 100% and
it should show 0% value when we have values [0, 0].
- With introduced "breaking" change in d3-scale, it returns middle point
for range [0, 0] and it equals 50% (for range0 from % to 100%).
- With fix and as it was before, for 0 value scaling within [0, 0] domain
will return 0%
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

@koorosh if the goal is to explicitly bring back support for the "0" graph, can't we just choose not to render a graph at all when the values are zero? I feel like there should be a better solution for the case when we don't want a graph to render. Doesn't d3 support this more directly?

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @dhartunian and @elkmaster)

Copy link
Contributor Author

@koorosh koorosh left a comment

Choose a reason for hiding this comment

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

I don't feel that it's a good option to conditionally render or not to render component, because it has to show label with bar as well (0% [ empty bar ]).
D3 isn't responsible for rendering elements in our case so we can't rely on this as well. D3 only provides us utility function to scale the value proportionally to barCharts width.

Before, scaling behavior wasn't defined for cases like this and it worked the way we needed, but now additional checks have to be done.

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @elkmaster)

Copy link
Contributor

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm: thanks for looking into this. Interesting problem. I don't see an easy way to avoid introducing this helper function.

Reviewed 7 of 7 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @elkmaster)

@koorosh koorosh merged commit 72ff0ad into cockroachdb:master Nov 25, 2020
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.

ui: barChart is 50% filled for zero values
3 participants