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

Dynamic Y-Axis Sizing #3749

Merged
merged 9 commits into from
May 12, 2015
Merged

Dynamic Y-Axis Sizing #3749

merged 9 commits into from
May 12, 2015

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented May 4, 2015

Closes #2693

  • Adjust the width of the y-axis based on the label content
  • Render x-axis and chart after the y-axis, so spacing is correctly calculated

after the axis labels have been rendered and the size of the element can be checked
@w33ble
Copy link
Contributor Author

w33ble commented May 4, 2015

@stormpython once reviewed, please please pass it to someone else to get a second set of eyes on it.

so that the x-axis sizing is done against the adjusted size
@stormpython
Copy link
Contributor

@w33ble I found two issues. The first happens when numbers grow larger than 1 million, and the second happens when labels along the x-axis are rotated. See screenshots below.

screen shot 2015-05-05 at 9 57 08 am
The above image is missing the y-axis line. The path is drawn on the screen, but its pushed behind the chart svg so it is not visible.

screen shot 2015-05-05 at 9 58 29 am
Here you can see the issue above as well as an issue with where the x-axis line is drawn compared to the bars

@stormpython stormpython assigned w33ble and unassigned stormpython May 5, 2015
@stormpython stormpython removed the review label May 5, 2015
@w33ble w33ble assigned stormpython and unassigned w33ble May 11, 2015
@w33ble
Copy link
Contributor Author

w33ble commented May 11, 2015

@stormpython should be in better shape now. I'm assigning the width as part of the layout, using a rendered version of the y-axis to get the width at that step. Take another look and let me know if you see issues.

@stormpython
Copy link
Contributor

@w33ble LGTM! I am going to assign another person to review this just to make sure I didn't miss something.

@lukasolson
Copy link
Member

LGT:m:!

lukasolson added a commit that referenced this pull request May 12, 2015
@lukasolson lukasolson merged commit d029b34 into elastic:master May 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flexible y-axis styling
4 participants