michael-s-molina commented on code in PR #33517:
URL: https://github.com/apache/superset/pull/33517#discussion_r2096356005
##########
superset-frontend/packages/superset-ui-core/src/chart/types/VizType.ts:
##########
@@ -54,6 +54,7 @@ export enum VizType {
Step = 'echarts_timeseries_step',
Sunburst = 'sunburst_v2',
Table = 'table',
+ TableAgGrid = 'table_ag_grid',
Review Comment:
> We do need seperate visualisations since the original table has alot of
time comparison features & already so much legacy code in place that rewriting
the entire legacy table with ag grid can break the backward compatibility for
existing clients , it might be possible in future to completely migrate to ag
grid & discard the old table but as of now , i think this carries risk seeing
the complexity of existing table chart , given that table is one of the most
widely used charts
Thanks for the explanation. I agree that having separate visualizations with
a potential future migration might diminish the risk here.
> 3.I thought the same thing in the first place , but the thing is, it will
break the principle of loose coupling , sqllab & viz plugins have least common
functionalities & to implement any functionality in viz plugin table v2 might
cause implication in sqllab component where i have to keep on passing flags to
check where isSqllab or isVizPlugin , so i implemented it from scratch so that
its losely coupled & any changes in this component doesnt break in sqllab or
sqlllab changes doesnt break viz plugin
Supporting different implementations will be problematic from a maintenance
and user experience perspective. We should try to extract common functionality
to a shareable component that can be extended to handle custom needs of SQL Lab
and chart visualizations. Please collaborate with @justinpark on how we can
reuse as much code as possible. He's the author of [[SIP-123] Proposal
replacement of data table components with
ag-grid](https://github.com/apache/superset/issues/27645) which is related with
this work.
##########
superset-frontend/packages/superset-ui-core/src/chart/types/VizType.ts:
##########
@@ -54,6 +54,7 @@ export enum VizType {
Step = 'echarts_timeseries_step',
Sunburst = 'sunburst_v2',
Table = 'table',
+ TableAgGrid = 'table_ag_grid',
Review Comment:
> We do need seperate visualisations since the original table has alot of
time comparison features & already so much legacy code in place that rewriting
the entire legacy table with ag grid can break the backward compatibility for
existing clients , it might be possible in future to completely migrate to ag
grid & discard the old table but as of now , i think this carries risk seeing
the complexity of existing table chart , given that table is one of the most
widely used charts
Thanks for the explanation. I agree that having separate visualizations with
a potential future migration might diminish the risk here.
> 3.I thought the same thing in the first place , but the thing is, it will
break the principle of loose coupling , sqllab & viz plugins have least common
functionalities & to implement any functionality in viz plugin table v2 might
cause implication in sqllab component where i have to keep on passing flags to
check where isSqllab or isVizPlugin , so i implemented it from scratch so that
its losely coupled & any changes in this component doesnt break in sqllab or
sqlllab changes doesnt break viz plugin
Supporting different implementations will be problematic from a maintenance
and user experience perspective. We should try to extract common functionality
to a shareable component that can be extended to handle custom needs of SQL Lab
and chart visualizations. Please collaborate with @justinpark on how we can
reuse as much code as possible. He's the author of [[SIP-123] Proposal
replacement of data table components with
ag-grid](https://github.com/apache/superset/issues/27645).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
