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

[ML] Fix cosmetic issues with cut off chart overflows. #19794

Merged
merged 2 commits into from Jun 11, 2018

Conversation

Projects
None yet
4 participants
@walterra
Copy link
Contributor

commented Jun 11, 2018

Fixes #18023.

  1. Increases the top margin to 5 and the right margin to 1 to avoid cutting off the chart because overflow is hidden for the directives mlEventRateChart and mlMultiMetricJobChart. The top margin gives enough room to avoid cutting off y-axis labels. The right margin is a tweak to not cut off the gray border by 1 pixel to the right.

Before and after:

  1. Fixes how the domain for mlEventRateChart is calculated. The domain gets now extended by 1 barsInterval, otherwise the last bar will start at the end of vizWidth and overflow the chart (the overflow is hidden so the visible chart missed one bar).

The following screenshot demonstrates the issue by temporarily setting overflow: visible:

image

The fixed version adjusts the domain and the last bar doesn't overflow the chart anymore:

image

@elasticmachine

This comment has been minimized.

Copy link

commented Jun 11, 2018

Pinging @elastic/ml-ui

@peteharverson
Copy link
Contributor

left a comment

LGTM, just one minor typo!

@@ -120,9 +120,15 @@ module.directive('mlEventRateChart', function () {
}
swimlaneXScale.domain(d3.extent(finerData, d => d.date));


// Extent the time range/domain at the end by 1 barsInterval,

This comment has been minimized.

Copy link
@peteharverson

peteharverson Jun 11, 2018

Contributor

Should be 'Extend' rather than 'Extent' !

@jgowdyelastic
Copy link
Member

left a comment

LGTM

@elasticmachine

This comment has been minimized.

Copy link

commented Jun 11, 2018

@walterra

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

jenkins retest this

@elasticmachine

This comment has been minimized.

Copy link

commented Jun 11, 2018

@walterra walterra merged commit d2e0aed into elastic:master Jun 11, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

walterra added a commit to walterra/kibana that referenced this pull request Jun 11, 2018

[ML] Fix cosmetic issues with cut off chart overflows. (elastic#19794)
1. Increases the top margin to 5 and the right margin to 1 to avoid cutting off the chart because overflow is hidden for the directives mlEventRateChart and mlMultiMetricJobChart. The top margin gives enough room to avoid cutting off y-axis labels. The right margin is a tweak to not cut off the gray border by 1 pixel to the right.
2. Fixes how the domain for mlEventRateChart is calculated. The domain gets now extended by 1 barsInterval, otherwise the last bar will start at the end of vizWidth and overflow the chart (the overflow is hidden so the visible chart missed one bar).

walterra added a commit that referenced this pull request Jun 12, 2018

[ML] Fix cosmetic issues with cut off chart overflows. (#19794) (#19801)
1. Increases the top margin to 5 and the right margin to 1 to avoid cutting off the chart because overflow is hidden for the directives mlEventRateChart and mlMultiMetricJobChart. The top margin gives enough room to avoid cutting off y-axis labels. The right margin is a tweak to not cut off the gray border by 1 pixel to the right.
2. Fixes how the domain for mlEventRateChart is calculated. The domain gets now extended by 1 barsInterval, otherwise the last bar will start at the end of vizWidth and overflow the chart (the overflow is hidden so the visible chart missed one bar).

@walterra walterra deleted the walterra:ml-chart-axis-overflow-fix branch Jun 12, 2018

maryia-lapata added a commit to maryia-lapata/kibana that referenced this pull request Jun 25, 2018

[ML] Fix cosmetic issues with cut off chart overflows. (elastic#19794)
1. Increases the top margin to 5 and the right margin to 1 to avoid cutting off the chart because overflow is hidden for the directives mlEventRateChart and mlMultiMetricJobChart. The top margin gives enough room to avoid cutting off y-axis labels. The right margin is a tweak to not cut off the gray border by 1 pixel to the right.
2. Fixes how the domain for mlEventRateChart is calculated. The domain gets now extended by 1 barsInterval, otherwise the last bar will start at the end of vizWidth and overflow the chart (the overflow is hidden so the visible chart missed one bar).

walterra added a commit that referenced this pull request Jul 11, 2018

[ML] Fixes a regression bug which made the progress bar invisible. (#…
…20618)

Fixes a regression introduced by #19794. This PR added a 5px top margin to the affected charts. The hard coded bottom margin of the progress bars wasn't adjusted accordingly so the progress bars ended up being hidden behind the actual chart. This PR fixes the problem by taking the chart's top margin into account for progressBarMarginBottom.

walterra added a commit to walterra/kibana that referenced this pull request Jul 11, 2018

[ML] Fixes a regression bug which made the progress bar invisible. (e…
…lastic#20618)

Fixes a regression introduced by elastic#19794. This PR added a 5px top margin to the affected charts. The hard coded bottom margin of the progress bars wasn't adjusted accordingly so the progress bars ended up being hidden behind the actual chart. This PR fixes the problem by taking the chart's top margin into account for progressBarMarginBottom.

walterra added a commit that referenced this pull request Jul 12, 2018

[ML] Fixes a regression bug which made the progress bar invisible. (#…
…20618) (#20670)

Fixes a regression introduced by #19794. This PR added a 5px top margin to the affected charts. The hard coded bottom margin of the progress bars wasn't adjusted accordingly so the progress bars ended up being hidden behind the actual chart. This PR fixes the problem by taking the chart's top margin into account for progressBarMarginBottom.
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.