Re: [PR] feat: Tooltip renderer [superset]

[via GitHub]

michael-s-molina commented on PR #27950:
URL: https://github.com/apache/superset/pull/27950#issuecomment-2047454607

   @justinpark @villebro @rusackas Thank you for the comments. I'm still 
working on the component's API and implementation as I continue to iterate on 
each plugin. I'm still not sure what will be the final API and implementation 
because each plugin has a unique set of requirements. For that reason, I'll 
first first make sure all ECharts plugins are using the renderer, to have a 
clear view of all the requirements, and then work on optimizations. Feel free 
to review this PR while it evolves or wait for when it leaves the draft state.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org

Re: [PR] feat: Tooltip renderer [superset]

[via GitHub]

villebro commented on code in PR #27950:
URL: https://github.com/apache/superset/pull/27950#discussion_r1558158991


##
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##
@@ -249,7 +229,27 @@ export default function transformProps(
   ...getDefaultTooltip(refs),
   show: !inContextMenu,
   trigger: 'axis',
-  formatter: renderTooltipFactory(formatTime, headerFormatter),
+  formatter: (params: { data: TimeSeriesDatum }[]) => {
+const data: Data = {
+  columns: [
+{ type: GenericDataType.String, formatter: String },
+{ type: GenericDataType.Numeric, formatter: headerFormatter },
+  ],
+  rows: [
+[
+  metricName,
+  params[0].data[1] === null ? t('N/A') : params[0].data[1],
+],
+  ],
+};
+return new TooltipRenderer(
+  data,
+  0,
+  false,
+  false,
+  formatTime(params[0].data[0]),
+).renderHtml();
+  },

Review Comment:
   It appears we're re-instantiating `TooltipRenderer` on each call to the 
formatter, which may (or may not) cause an unnecessary perf hit. Did you 
consider implementing this so that we can define the renderer only once, 
essentially memoizing it, and then call that in a callback that is able to 
render the html using `params` as the input? Note, I may be way off here, but 
thinking out loud..



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org