[SIP-5] Refactor nvd3 #5838
[SIP-5] Refactor nvd3 #5838
Conversation
| }), | ||
| ]); | ||
|
|
||
| export const colorObjectType = PropTypes.shape({ |
There was a problem hiding this comment.
rgbObjectType might be a little more specific / clear.
| Q2: PropTypes.number, | ||
| Q3: PropTypes.number, | ||
| outliers: PropTypes.arrayOf(PropTypes.number), | ||
| whisker_high: PropTypes.number, |
There was a problem hiding this comment.
any use in camelCaseing this?
There was a problem hiding this comment.
Oh. It is nvd3's internal thing.
| ]); | ||
|
|
||
| export const colorObjectType = PropTypes.shape({ | ||
| r: PropTypes.number.isRequired, |
There was a problem hiding this comment.
how are you thinking about isRequired vs not? I'm not sure I have a strong opinion on this for now since just having types in general is a huge win here, mostly just curious.
There was a problem hiding this comment.
good point. There were a few cases where it mysteriously breaks because some particular fields were omitted and I spent too much time figuring out. After found the cause, I was upset and add isRequired as warning, but this was not thoroughly verified for every field.
| // 'pie' only | ||
| isDonut: PropTypes.bool, | ||
| isPieLabelOutside: PropTypes.bool, | ||
| pieLabelType: PropTypes.string, |
There was a problem hiding this comment.
I think this can be key, value, percent, key_value, or key_percent based on dropdown options. looks like key value and percent are supported by nvd3 and we have custom label generators for the others. do you think this is worth enumerating here?
| showLegend: PropTypes.bool, | ||
| showMarkers: PropTypes.bool, | ||
| useRichTooltip: PropTypes.bool, | ||
| vizType: PropTypes.string, |
There was a problem hiding this comment.
do you think it's worth enumerating types here?
| })), []); | ||
| .filter(layer => layer.show) | ||
| .filter(layer => layer.annotationType === AnnotationTypes.TIME_SERIES) | ||
| .reduce((bushel, a) => |
There was a problem hiding this comment.
ah, now I understand bushel 🤷♂️
|
|
||
| // on scroll, hide tooltips. throttle to only 4x/second. | ||
| $(window).scroll(throttle(hideTooltips, 250)); | ||
| window.addEventListener('scroll', throttle(hideTooltips, 250)); |
There was a problem hiding this comment.
wow, didn't know this was there. wonder if this is necessary since the tooltips go away quickly anyway.
There was a problem hiding this comment.
yeah, kinda surprise, but I will not touch this for now.
There was a problem hiding this comment.
I forgot the exact reason it's there, but would dig as deep as needed in bit blame to understand the reason before/if removing it.
There was a problem hiding this comment.
from #3518 by chart annotation feature.
|
You get a 🏆 for even attempting the refactor of this file. I just saw that the now main maintainer of nvd3 has forked the project project to take it beyond |
|
Yay! CI is green. |
|
Release the Kraken! 🦑 |
|
merging, big improvement! |
* move into folder and scaffold adaptor * extract width and height * remove jquery * extract showBrush * extract lineInterpolation * extract xAxisFormat and yAxisFormat * extract annotationData * extract xTicksLayout and colorScheme * extract showXXX * extract x and y axis labels * extract showminmax * extract pie chart props * extract area chart props * extract logscale and yAxisBounds * extract margin * extract bubble props x,y,size * extract contribution, comparisonType and color_picker * remove the last fd.xxx * remove unnecessary variables * remove slice.container * fix unit test reference * Rewrite logic to compute max label lengths to use only d3, not jquery. * extract annotationLayers and no more slice.xxx in nvd3vis * rename x, y, size to xField, yField, sizeField * use arrow function * move tooltip function * extract helper functions into utils * remove unused argument * fix height calculation and show bar labels * rename function * update unit test * organize tooltip generator * update line_multi * Add proptypes for data * list proptypes for data * extract tooltip function for bubble chart * rename variables * remove console.log * enumerate vizTypes and pieLabelType * parse maxBubble * use new color scale * fix import" * remove line
mistercrunch
added
🏷️ bot
sliceandformDataPropTypes@williaster @conglei @graceguo-supercat