Add annotation information to ChartMetadata #6136
Conversation
| show = true, | ||
| canBeAnnotation = [], |
There was a problem hiding this comment.
canBeAnnotationTypes / supportedAnnotationTypes might be marginally clearer, but I'm okay with canBeAnnotation / supportedAnnotations, too.
if going with canBeAnnotation / supportedAnnotations, maybe we should also make canBeAnnotation plural for consistency? => canBeAnnotations?
There was a problem hiding this comment.
I like canBeAnnotationTypes / supportedAnnotationTypes. Was overthinking about singular/plural thing. I think your suggestion makes sense.
| lookup[type] = true; | ||
| return lookup; | ||
| }, {}); | ||
| this.supportedAnnotations = supportedAnnotations; |
There was a problem hiding this comment.
any reason to not store these with the same data structure? (don't have strong opinion, mostly curious how you were thinking about it)
There was a problem hiding this comment.
I have same concern about storing as different data structure too. The benefit is it is better for performance.
When a user select annotation type TIME_SERIES, the ui need to loop through all vis to figure out which vis can be used as annotation source for TIME_SERIES in addition to NATIVE source. An object can provide immediate lookup with better perf than array. Although it is a very small array so doesn't really matter much.
Can store it as another field name to distinguish. Not sure what would be a good name.
Codecov Report
@@ Coverage Diff @@
## master #6136 +/- ##
=======================================
Coverage 76.91% 76.91%
=======================================
Files 47 47
Lines 9362 9362
=======================================
Hits 7201 7201
Misses 2161 2161Continue to review full report at Codecov.
|
* add annotations information to ChartMetadata * rename fields
mistercrunch
added
🏷️ bot
Add annotation information to ChartMetadata
canBeAnnotation: List the annotation types that the chart can be use as a source.supportedAnnotations: List the annotation types that can be added to this chart.@williaster @graceguo-supercat @conglei @michellethomas