Conversation
timroes
left a comment
There was a problem hiding this comment.
Just some minor ideas, but besides those LGTM, and I let you decide on these ideas.
|
|
||
| <div class="kuiSideBarFormRow"> | ||
| <label class="kuiSideBarFormRow__label" for="truncateLabels"> | ||
| Truncate |
There was a problem hiding this comment.
I feel like "Truncate" is a bit ambiguous. I first thought it's the amount of letters, but it seems to be the pixel width after which a label is truncated. Could we maybe change this to "Maximum (Label) Width" and maybe add a "(px)" (behind the label or the textbox) to make clear in what unit this is?
@alexfrancoeur your thoughts on this?
| while (str[endChar - 1] === ' ' || str[endChar - 1] === '-' || str[endChar - 1] === ',') { | ||
| endChar = endChar - 1; | ||
| } | ||
| str = str.substr(0, endChar) + '...'; |
There was a problem hiding this comment.
We could use the proper ellipsis char (…) instead of three dots here.
| .append('text') | ||
| .text(function (d) { | ||
| if (d.depth === 0) { | ||
| return; |
There was a problem hiding this comment.
If you add this.parentElement.remove() here, that wouldn't leave an empty g element for the actual inner donut circle.
alexfrancoeur
left a comment
There was a problem hiding this comment.
@ppisljar great work! Some (late 😦 ) feedback below:
Upon landing on an empty pie chart (no data in the timeframe), I'm immediately presented with an empty count. Maybe data labels should be off by default? This seems a bit odd to me.
When there is data, this also seems a bit odd.
Does truncate work? when adding 10 characters I see nothing. When adding 50 I see half. 100 shows all. Is this a percentage? Normally this is done by characters, can you confirm?
I don't care too much either way but is Show Top Level Only more intuitive then Show Only on Last Level?
There are two values associated with every slice of a pie. Should we provide a checkbox for both options? Show percentage and Show value? I'm not sure we can show both at the same time, but I imagine this being a request we'll hear shortly after merging this.
Sorry for the late review, hope this helps.
|
closing in favour of #12174 |
Fixed merge errors of #12174.
This adds label to pie-charts.