[GitHub] [superset] eric-briscoe commented on pull request #22135: feat: Adds virtualization option to antd based Table component

[GitBox <gi...@apache.org>]

eric-briscoe commented on PR #22135:
URL: https://github.com/apache/superset/pull/22135#issuecomment-1320424423

   > Hello @eric-briscoe. Thanks for this PR! I have some questions about VirtualTable:
   > 
   > * There's `react-virtual` (5.3 kB) which 3 times less than `rc-resize-observer` (15.6 kB) and provides hook for this func component. Should we use `react-virtual` for this VirtualTable?
   > * Why do we use `antd based Table` instead of `react-table`. `react-table` already supports virtualization - [virtualized-infinite-scrolling](https://github.com/TanStack/table/blob/main/examples/react/virtualized-infinite-scrolling/src/main.tsx) and [virtualized-rows](https://github.com/TanStack/table/blob/main/examples/react/virtualized-rows/src/main.tsx)
   > * Can we also use the VirtualTable in [FilterableTable](https://github.com/apache/superset/blob/master/superset-frontend/src/components/FilterableTable/index.tsx#L693) for result data in SQL Lab
   
   @EugeneTorap Great questions!
   
   @michael-s-molina noted in a diff comment on this PR we already have 'react-resize-detector' as a dependency used in components such as MetadataBar so I refactored to use that library in place of `rc-resize-observer` and this PR introduces no new dependencies now
   
   Bigger picture, the introduction of src/components/Table based on antd is continuing of work started with https://github.com/apache/superset/issues/20159 (SIP-82) about more consistent theming, use of Ant Design components and establishing a formal design system using antd as a base for new components. 
   
   VitrualTable is never intended to be used directly.  It is a sub component to enable the src/components/Table to operate in a virtualized mode if needed and implementation is based from this antd example, then modified for our needs: https://ant.design/components/table/#components-table-demo-virtual-list
   
   If you check out PR and run storybook, you will see the documentation for using the new Table component, and in this PR docs on using the `virtualize` prop have been added.
   
   As far as using a different library such as react-table I was asked to use antd for consistency in a single Table implementation so that features like column definitions, sort functions, cell renderers, data formats, etc can be consistent, and the UI controls are consistent wether we are using the Table with or without virtualization.  
   
   Regarding use in SqlLab, the goal is to replace eventually replace all tables across superset-frontend with the new src/components/Table (but never VirtualTable directly).  Table uses VirtualTable as a sub component when the Table prop `virtualize` is set to true.  This allows of are lot of the common logic between virtual and non-virtual rendering to be handled at the Table component level and not duplicated in VirtualTable.
   
   I was asked to introduce the Table capability incrementally so that it can be used for the new dataset creation and editing workflows, fix issues in the drill to detail modal, and the target other high use areas of the app to start swapping out.  Some Table features will be need to be exposed / added as we do this and features refined as it gets used more.  I was also asked to make Table an abstraction to antd so that it can leverage antd components but streamline usage for Superset use cases and not require developer to understand / use antd directly.  That is bigger picture goal for design system which will likely be a follow on SIP to SIP-82, but still doing some research / prototyping to get this ready for community discussion.
   
   I would really appreciate more eyes on the storybook examples and documentation so we can improve it to make usage as clear and efficient as possible for all Superset contributors.  If you have any time to take a look and have feedback please send my way!


-- 
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