Chart Component: chart design feedback and updates

[greenafrican]

D3 Chart Component Master Thread: #164

Based on feedback from @LevinMedia - https://docs.google.com/document/d/1YuUh0kzaZ12ege3Lsi6JCB8FPN5uA3lx9Jlj0PB3qgE/edit

• If a legend is unchecked, it’s hover interaction should be disabled
• Add hover states to legends

Chart

• Ensure 16px of padding on left and right edges of chart area
• Left align text in y-axis ticks
• Add vertical pipes to x-axis ticks
• Add secondary label to x-axis (for month / year)
• Massage the rounding on y-axis labels
• Update zero (baseline) color

Tooltip

• The Tool-tip and vertical line disappear if you hover directly over a point on the chart
• Update styles to match design documentation

[greenafrican]

@LevinMedia Could you take a look at the todos from your list? I'll handle the rest in the next PR.

[Aljullu]

It works great! Looks good to me. 🙂

I added a couple of suggestions in the code.

The Tool-tip and vertical line disappear if you hover directly over a point on the chart

Out of curiosity, why was that decision taken?

[Aljullu]

This can be written like:
&.woocommerce-legend__item-checked button:hover {
Is there any reason to prefer nesting?

[Aljullu]

Should we use color instead of colour to be consistent with CSS properties?

[greenafrican]

Fixed in 70ab5a5

[Aljullu]

I think a comment here might help making this operation easier to understand.

Another solution would be splitting it into another function and adding a descriptive name to it:

const convertToPowerOf10 = value => {
	const pow3Y = Math.pow( 10, ( ( Math.log( value ) * Math.LOG10E + 1 ) | 0 ) - 2 ) * 3;
	return Math.ceil( value / pow3Y ) * pow3Y;
};

export const getYMax = lineData => {
	const yMax = 4 / 3 * d3Max( lineData, d => d3Max( d.values.map( date => date.value ) ) );
	return convertToPowerOf10( yMax );
};

(Probably a better name can be found)

What do you think? 🙂

I also think the Math.round() around yMax value can be removed, now.

[greenafrican]

Improved in 27c8537

[LevinMedia]

Looking good @greenafrican ! Per our convo this AM

Sorry for the confusion about the hover state / disable. The hover state on the legend should persist when a series is disabled - it's the behavior in the actual chart that should be disabled.

Add revenue symbols to y-axis legend, left align text.

Check ordering for the points on line - the tool tip is showing up consistently now, but the vertical line is still disappearing on hover directly over a point.

[timmyc]

Looking good to me as well - I am seeing some crowding on the axis labels, but I think that has already been noted:

pre-approving pending your feedback on @Aljullu notes

[timmyc]

🐞 i loathe ticks - reading it makes me shudder 😆

[greenafrican]

Sorry for the confusion about the hover state / disable. The hover state on the legend should persist when a series is disabled - it's the behavior in the actual chart that should be disabled.

• Fixed in 48b3456

[greenafrican]

Add revenue symbols to y-axis legend

This may be an issue. The d3 format specifier that massages the number into an SI-Prefix (M for millions etc.) isn't compatible with the Intl.NumberFormatter at the moment. It seems as though there may be an effort to support this here - tc39/ecma402#37

I can probably write a manual specifier, which I could extract into the props.

[greenafrican]

...legend, left align text.

• Fixed in 8ec8442

[greenafrican]

vertical line is still disappearing on hover directly over a point.

• Fixed in 6dec48b

[greenafrican]

Add revenue symbols

• Fixed in adc61d8 and 9a3f163