-
Notifications
You must be signed in to change notification settings - Fork 77
feat: update line chart tooltip and use selector #31
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a couple comments, looks good overall!
@@ -1,23 +1,28 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this file not generated by beemo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... true. The one in demo
is somehow not generated and is also checked in.
value: encoder.channels.y.formatValue(series[key].y), | ||
keyColumn: ( | ||
<> | ||
<svg width="12" height="8" style={MARK_STYLE}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this glyph not match the legend? that seems ideal. it's tricky with the dashed line, tho. In that case I wonder if we should update the legend to be lines instead of circles or squares. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps should update the legend to accept mark type and display line as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making the legend customizable seems like a good chunk of work itself. I will make it a follow-up PR.
keyStyle?: CSSProperties; | ||
value: string | number; | ||
valueColumn: ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there cases where this is not string/number/null
? ReactNode
seems strange to have inside an array of data objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to be flexible in case extra formatting is needed or there are unforeseen thing we want to put in the value column. Also to make it same with keyColumn
* feat: Add number-format package
🏆 Enhancements
selector
to avoid recreatingEncoder