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

Missing bottom padding for gauge charts legend #1441

Closed
RonnyWeiss opened this issue Jun 12, 2020 · 10 comments
Closed

Missing bottom padding for gauge charts legend #1441

RonnyWeiss opened this issue Jun 12, 2020 · 10 comments
Labels
Milestone

Comments

@RonnyWeiss
Copy link

Hello

When using Gauges with Legends then therese no padding at the bottom of the legend. The others charts types have the padding at the bottom.

Gauge:
gauge

Line:
line

Would be very cool if you could fix that.

Kind regards and a nice weekend

Ronny

@netil netil added the request label Jun 15, 2020
@watnab
Copy link

watnab commented Jun 18, 2020

} else if (target === "legend") {
x = $$.margin3.left;
y = $$.margin3.top + (hasGauge ? 10 : 0);

@michkami
Copy link
Collaborator

} else if (target === "legend") {
x = $$.margin3.left;
y = $$.margin3.top + (hasGauge ? 10 : 0);

This code is my fault. I added it to create a gap between legend and gauge labels, as there was no padding between them.
As far I can see 10 pixel seem to be too much, it should be rather 4 pixel (same as between gauge label and gauge) gap to make it look more symetric.

I actually don't think that the legend is the only problem for the missing padding at the bottom of the chart.
If you remove + (hasGauge ? 10 : 0) there will be bottom padding if the legend is enabled and positioned at the bottom, but if you set legend_show = false or/and gauge_labels_show = false } gauge still won't have padding at the bottom.

By removing + (hasGauge ? 10 : 0) you can see, that the legend container does not render at the end od the gauge label container but in the middle of them.
grafik

The reason for this could be that the starting y coordinate for gauge labels is placed in the arc (not outside like you can see in the screenshot below) and is than moved by

.attr("dy", "1.2em")
to the final position under the arc.
grafik

But as I wrote above, adding margin to the legend won't fix the issue for all use cases.

@watnab
Copy link

watnab commented Jun 18, 2020

It seems g.bb-chart-arc(the content of g.bb-chart) of the gauge should be a bit smaller.
However I could not find which function calculate the height.
gauge
donut
radar
I added the red border to the svg element.

@michkami
Copy link
Collaborator

michkami commented Jun 19, 2020

@watnab gauge height is calculated here:

// for arc
$$.arcWidth = $$.width - ($$.isLegendRight ? legend.width + 10 : 0);
$$.arcHeight = $$.height - ($$.isLegendRight ? 0 : 10);
if ($$.hasType("gauge") && !config.gauge_fullCircle) {
$$.arcHeight += $$.height - $$.getGaugeLabelHeight();
}

I tried few things and got following results:

Description Single Multi
Labels: true && Legend: bottom single-labelsTrue-legendBottom multi-labelsTrue-legendBottom
Labels: true && Legend: right single-labelsTrue-legendRight multi-labelsTrue-legendRight
Labels: false && Legend: bottom single-labelsFalse-legendBottom multi-labelsFalse-legendBottom
Labels: false && Legend: right single-labelsFalse-legendRight multi-labelsFalse-legendRight
Labels: false && Legend: false single-labelsFalse-legendFalse multi-labelsFalse-legendFalse

I removed + (hasGauge ? 10 : 0) from:

} else if (target === "legend") {
	x = $$.margin3.left;
	y = $$.margin3.top;
}

I added following function to arc.js:

getPaddingBottomForGauge() {
	const $$ = this;
	const defaultGaugeLabelHeight = 20; // value returned by 'getGaugeLabelHeight' if 'label_show: true'
	const legendShow = $$.config.legend_show;
	const gaugeLabelShow = $$.config.gauge_label_show;

	if (legendShow && gaugeLabelShow) {
		return $$.getGaugeLabelHeight() * ($$.isLegendRight ? 1.5 : 1);
	} else if (legendShow && !gaugeLabelShow) {
		return defaultGaugeLabelHeight * 1.5;
	} else if (!legendShow && gaugeLabelShow) {
		return $$.getGaugeLabelHeight() * 1.5;
	} else {
		return defaultGaugeLabelHeight * 1.5;
	}

changed the arcHeight calculation to this:

$$.arcHeight = $$.height - ($$.hasType("gauge") ? $$.getPaddingBottomForGauge() : ($$.isLegendRight ? 0 : 10));

It works for all gauge types, and it's only breaking 1 gauge test which can be easily updated.

If this solution is ok for @netil I could make a pull request.

@netil
Copy link
Member

netil commented Jun 24, 2020

@michkami, can you make PR to v2 branch?

@netil
Copy link
Member

netil commented Jun 25, 2020

From the PR (the right screenshot), will make have padding at the bottom.

image

But after the update, size of the gauge itself will become more smaller than before.
The differences can be compared with the screenshot.

  • left: current / right: PR update

@RonnyWeiss
Copy link
Author

I think this is ok! Another solution would be to position the legend more to the value title so gauge does not get smaller. But this is up to you how you handle that. The current workaroud of mine was also to make the gauge little bit smaller in the height and to put a small padding outside of the svg

@michkami
Copy link
Collaborator

I think this is ok! Another solution would be to position the legend more to the value title so gauge does not get smaller. But this is up to you how you handle that. The current workaroud of mine was also to make the gauge little bit smaller in the height and to put a small padding outside of the svg

Positioning the legend closer to value would overlap gauge min and gauge max as you can have multiple data columns:
grafik
grafik

@netil I think it is necessary to make the gauge a bit smaller, as the gauge and the legend are rendered into the same container.

@RonnyWeiss
Copy link
Author

RonnyWeiss commented Jun 25, 2020

Yes you're right! So making the gauge smaller would be the best solution! Thank you very much for fixing that!

Kind regards

Ronny

netil pushed a commit that referenced this issue Jun 25, 2020
from now 'isLegendRight' can be truthy only if legend is enabled

Fix #1441
Close #1471
@netil netil added this to the 2.0-next milestone Jun 25, 2020
@netil netil added bug and removed request labels Jun 25, 2020
@netil
Copy link
Member

netil commented Jun 25, 2020

thanks @RonnyWeiss, @michkami for the contribution!
The changes will be in the next RC(next) release. :)

@netil netil closed this as completed Jun 25, 2020
netil pushed a commit that referenced this issue Jun 26, 2020
# [2.0.0-next.7](2.0.0-next.6...2.0.0-next.7) (2020-06-26)

### Bug Fixes

* **gauge:** fixed wrong bottom padding calculation ([0542586](0542586)), closes [#1441](#1441) [#1471](#1471)
netil pushed a commit that referenced this issue Jul 16, 2020
# [2.0.0](1.12.11...2.0.0) (2020-07-16)

### Bug Fixes

* **all:** Fix test cases ([2e1ad79](2e1ad79))
* **arc:** fix applying data.labels.colors ([#1448](#1448)) ([c128fad](c128fad)), closes [#1440](#1440)
* **axis:** fix incorrect clip node handling ([a8c6f96](a8c6f96)), closes [#1449](#1449)
* **axis:** make axis clip-path to fit real axis size ([7419f44](7419f44)), closes [#1449](#1449)
* **bar:** fix bar width scale on zoom ([59073bd](59073bd)), closes [#1476](#1476)
* **data:** fix for data.labels=false ([b7a0972](b7a0972)), closes [#1444](#1444)
* **data.selection:** fix selection.isselectable value check ([9d41a04](9d41a04))
* **gauge:** fixed wrong bottom padding calculation ([0542586](0542586)), closes [#1441](#1441) [#1471](#1471)
* **legend:** Don't bind event when interaction is false ([4546c00](4546c00))
* **point:** Correct focus.only to work in mobile env ([67eea16](67eea16))
* **point:** Correct point.focus.only ([1686594](1686594))
* **point:** update point generation ([da63e39](da63e39))
* **subchar:** correct subchart rendering ([44ed216](44ed216)), closes [#1458](#1458)

### Code Refactoring

* **all:** v2 updates ([e23998f](e23998f)), closes [#758](#758) [#757](#757) [#756](#756) [#36](#36)
* **module:** implement ESM index ([85caf71](85caf71))

### Features

* **axis:** Intent to ship log scale  ([6fdf3e4](6fdf3e4)), closes [#1351](#1351)
* **bar:** Intent to ship bar.label.threshold ([72a7b7f](72a7b7f)), closes [#1427](#1427)
* **gauge:** Intent to ship gauge.label.threshold ([#1443](#1443)) ([9a0807e](9a0807e)), closes [#1439](#1439)
* **interaction:** split selection, subchart & zoom ([ba1e4f2](ba1e4f2))
* **point:** Intent to ship point.focus.only ([bb70347](bb70347))

### BREAKING CHANGES

* **all:** v2 updates
* **module:** new index for ESM build

- split Axis releated size from size.ts --> size.axis.ts
- split common main option from Options.ts --> ./common/main.ts
- Instead export Axis class, add .getAxisInstance() to make beneficial
  from tree-shaking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants