-
Notifications
You must be signed in to change notification settings - Fork 14.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[time table] add tooltip to sparkline #3767
[time table] add tooltip to sparkline #3767
Conversation
@@ -53,13 +54,12 @@ function viz(slice, payload) { | |||
} | |||
const tableData = metrics.map((metric) => { | |||
let leftCell; | |||
const context = Object.assign({}, fd, { metric }); | |||
const url = fd.url ? Mustache.render(fd.url, context) : null; |
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.
Why was Mustache removed? The url needs to be rendered with the context (to interpret something like {{ metric }}).
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.
ohhh, yeah I didn't understand what this was doing fully/thought it was forcing the url to be within the app. added it back.
this lgtm |
* [time table] add tooltip to sparkline * [time table] open link in new tab * [time table] add back Mustache
* [time table] add tooltip to sparkline * [time table] open link in new tab * [time table] add back Mustache
@mistercrunch @graceguo-supercat
This PR
@data-ui/sparkline
to0.0.47
where I added tooltip supportTested sparkline functionally (see screenshot) and tested url opening in new tab functionally for group by + non-group by combos, and with new mocha test