Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

feat: update line chart tooltip and use selector #31

Merged
merged 1 commit into from
Apr 1, 2019
Merged

Conversation

kristw
Copy link
Collaborator

@kristw kristw commented Mar 29, 2019

🏆 Enhancements

  • Use selector to avoid recreating Encoder
  • Update Line Chart tooltip to use line mark to indicate encoding and change text back to black.

image

image

@kristw kristw requested a review from a team as a code owner March 29, 2019 01:06
@kristw kristw changed the title feat: update tooltip and use selector feat: update line chart tooltip and use selector Mar 29, 2019
Copy link
Contributor

@williaster williaster left a 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 @@
{
Copy link
Contributor

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?

Copy link
Collaborator Author

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}>
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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;
Copy link
Contributor

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.

Copy link
Collaborator Author

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

@kristw kristw merged commit 6a59b21 into master Apr 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the kristw--line4 branch April 1, 2019 20:56
nytai referenced this pull request in preset-io/superset-ui-plugins Apr 27, 2020
* feat: Add number-format package
nytai referenced this pull request in preset-io/superset-ui-plugins Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants