[GitHub] [superset] villebro commented on a diff in pull request #23698: feat: format timestamps in drill by breadcrumbs

["villebro (via GitHub)" <gi...@apache.org>]

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


##########
superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js:
##########
@@ -187,6 +192,10 @@ function WorldMap(element, props) {
           col: entity,
           op: '==',
           val,
+          formattedVal:
+            getColumnLabel(entity) === formData.granularitySqla
+              ? String(timestampFormatter(val))
+              : String(val),

Review Comment:
   While this will probably work well in legacy charts that still have a time section, this won't work for "modern" charts that don't have a time section when `GENERIC_CHART_AXES` is enabled. For this chart this might be the correct solution, as I don't remember if `viz.py` returns `colnames` (=all columns) and `coltypes` (=the respective datatypes for said columns), but at least for charts that are powered by the v1 chart API we should check if the datatype of the column is `GenericDataType.TEMPORAL`, and if so, format it with `timestampFormatter`.



##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:
##########
@@ -167,6 +171,10 @@ export default function EchartsMixedTimeseries({
               col: dimension,
               op: '==',
               val: values[i],
+              formattedVal:
+                DTTM_ALIAS === getColumnLabel(dimension)
+                  ? String(timestampFormatter(values[i]))
+                  : String(values[i]),

Review Comment:
   same here, `DTTM_ALIAS` is already mostly deprecated, so we should refrain from using it in these types of checks. Again, this might be the correct solution for legacy charts that are powered by `viz.py`, but in the ECharts plugin we should not be using `DTTM_ALIAS` anymore.



-- 
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-unsubscribe@superset.apache.org

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


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