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

Make vertical bar chart gutter widths consistent #12264

Merged
merged 4 commits into from Jun 21, 2017

Conversation

Projects
None yet
5 participants
@nreese
Copy link
Contributor

commented Jun 9, 2017

fixes #3230

Release Note:
Histogram intervals are not always equal widths (monthly time intervals). Keeping gutter widths consistent and reducing the bar width provides a more visually appealing bar chart.

Before - notice gutter widths visually different between January and February
screen shot 2017-06-09 at 10 57 38 am

After - gutter widths are the same for all bars
screen shot 2017-06-09 at 1 22 04 pm

@nreese nreese requested review from ppisljar and lukasolson Jun 9, 2017

@ppisljar
Copy link
Member

left a comment

you should do the same for grouped bars as well (there is another widthFunc lower in the same file). nice solution!

@nreese

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2017

@ppisljar Can I remove groupCount from addStackedBars? It seems like the value for groupCount will always be 1 for stacked bars and groupCount only exists in addStackedBars because at some point in the past, the functionality between addStackedBars and addGroupedBars shared code.

}

function x(d) {
/**
* Histogram intervals are not always equal widths, e.g, monthly time intervals.

This comment has been minimized.

Copy link
@lukasolson

lukasolson Jun 13, 2017

Member

Not a big fan of the duplicated code here. Is there some way to pull this out into a separate function? (I know they're not identical but they're close enough that they should be pulled into a separate module.)

This comment has been minimized.

Copy link
@nreese

nreese Jun 13, 2017

Author Contributor

yah, I was tried creating a common function. The problem is that if you are outside of the scope of addStackedBars or addGroupedBars then the number of arguments balloons and the function loses readability since it is mainly just a bunch of arguments.

@ppisljar
Copy link
Member

left a comment

LGTM

@lukasolson
Copy link
Member

left a comment

LGTM!

@nreese nreese force-pushed the nreese:gutter branch from cf312da to 6cf9981 Jun 20, 2017

@nreese nreese merged commit 57f7da6 into elastic:master Jun 21, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details
@jimgoodwin

This comment has been minimized.

Copy link

commented Jul 25, 2017

@nreese Release Note: paragraph in the description please

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.