feat(ebay-donut-chart): implement donut chart with highcharts #2140
Conversation
🦋 Changeset detectedLatest commit: 2730e68 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| .tooltip-pointer { | ||
| position: absolute; | ||
| width: 0; | ||
| height: 0; | ||
| border-left: 5px solid transparent; | ||
| border-right: 5px solid transparent; | ||
| border-bottom: 5px solid var(--color-black); | ||
| } |
There was a problem hiding this comment.
Just wondering, why is this overriding the tooltip values?
There was a problem hiding this comment.
Ah, this is leftover from when I was overriding highcharts default styling (which has no pointer)
| <span class="donut-tooltip tooltip tooltip--expanded"> | ||
| <div class="tooltip__overlay"> | ||
| <span class="tooltip__pointer tooltip__pointer--bottom"/> | ||
| <div class="tooltip__mask"> | ||
| <div class="tooltip__cell"> | ||
| <div class="tooltip__content"> | ||
| <b>${input.name}</b> | ||
| <br> | ||
| <if(input.tooltip)> | ||
| ${input.tooltip} | ||
| </if> | ||
| <else>${input.value}</else> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </span> |
There was a problem hiding this comment.
For this, can we not just use ebay-tooltip and pass in open attribute
There was a problem hiding this comment.
I tried to do this, but from my testing, setting ebay-tooltip with open as true does not display on first page render. It also appears to still be a controlled value in the demos, this could be a potential bug?
There was a problem hiding this comment.
Maybe just go with taking the BEM child element which should give you all the styles you need:
<div class="tooltip__overlay">
<span class="tooltip__pointer tooltip__pointer--bottom"/>
<div class="tooltip__mask">
<div class="tooltip__cell">
<div class="tooltip__content">
<b>${input.name}</b>
<br>
<if(input.tooltip)>
${input.tooltip}
</if>
<else>${input.value}</else>
</div>
</div>
</div>
</div>
| background: repeating-linear-gradient( | ||
| 30deg, | ||
| var(--color-data-viz-chart-tertiary-background), | ||
| var(--color-data-viz-chart-tertiary-background) 2px, | ||
| var(--color-data-viz-chart-tertiary-stroke) 2px, | ||
| var(--color-data-viz-chart-tertiary-stroke) 3px, | ||
| var(--color-data-viz-chart-tertiary-background) 3px, | ||
| var(--color-data-viz-chart-tertiary-background) 4px | ||
| ); |
There was a problem hiding this comment.
This could be a good candidate for a variable in skin
| <div | ||
| id=component.getContainerId() | ||
| class="ebay-donut-chart__graph-container" | ||
| no-update |
There was a problem hiding this comment.
Why is there no-update here?
There was a problem hiding this comment.
I noticed the state update (for active portion hover) was causing the highcharts to re-render and destroying the chart instance.
I can work on another solution, as no-update would cause issues if there's a need for data to update dynamically
| @@ -0,0 +1,72 @@ | |||
| .ebay-donut-chart__legend { | |||
There was a problem hiding this comment.
Do we need to add these styles in skin?
There was a problem hiding this comment.
Yes, seems like this could be in skin. Also wondering if this legend is specific to donut charts or could be generic for other types of chart? i.e. .chart-legend and .chart-legend__item instead of .donut-chart-legend and .donut-chart-legend__item?
| <div class="ebay-donut-chart__legend-label"> | ||
| <div class=[ | ||
| "ebay-donut-chart__legend-symbol", | ||
| `ebay-donut-chart__legend-symbol--${item.symbolClass}`, |
There was a problem hiding this comment.
Is this always defined?
If not, we might need to add a guard like
item.symbolClass && `ebay-donut-chart__legend-symbol--${item.symbolClass}\`,
| <div | ||
| class=[ | ||
| "ebay-donut-chart__legend-item", | ||
| input.focusIndex !== -1 && |
There was a problem hiding this comment.
Can we change this logic to be
!(input.focusIndex === index) && "ebay-donut-chart__legend-item--inactive"
I assume that index is not -1. If it can be, then maybe its fine as is
There was a problem hiding this comment.
Yea, using -1 is what I would consider an "old school" way of managing the null state of the focus index haha
So in this instance, none of these should have the inactive if there's no active index
I can update this logic to be a little better input.focusIndex !== null &&
|
@mikehobi Is the small version accounted for (with different layout)? I don't see it when running storybook. 
|
| } | ||
| } | ||
|
|
||
| <div class="ebay-donut-chart__legend"> |
There was a problem hiding this comment.
We should maybe leverage an HTML description list here?
There was a problem hiding this comment.
I did update this to use ul/li
It gets a little bit tricky using dl dt dd when styling, I could be wrong, but I'm not sure it allows for other elements to be nested within dl
@ianmcburnie I first assumed these were two "layout" types, but confirmed with Randy that these are breakpoint sizes, so narrowing the screen should show the small layout |
agliga
force-pushed
the
13.4.0
branch
4 times, most recently
from
609bc89
to
40f354c
Compare
agliga
force-pushed
the
13.4.0
branch
11 times, most recently
from
f9c329f
to
275bdac
Compare
Description
Context
References
Screenshots