Re: [PR] refactor(Table): Use our custom Table component in views [superset]

Mon, 07 Apr 2025 02:15:06 -0700 


korbit-ai[bot] commented on code in PR #32964:
URL: https://github.com/apache/superset/pull/32964#discussion_r2030776218


##########
superset-frontend/src/components/Table/index.tsx:
##########
@@ -50,9 +50,10 @@ export type OnChangeFunction<RecordType> =
 export enum TableSize {
   Small = 'small',
   Middle = 'middle',
+  Large = 'large',
 }

Review Comment:
   ### Missing Enum Documentation <sub>![category 
Documentation](https://img.shields.io/badge/Documentation-7c3aed)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The enum lacks documentation explaining its purpose and usage.
   
   ###### Why this matters
   Without context, developers may not understand when to use different table 
sizes or their visual impact.
   
   ###### Suggested change ∙ *Feature Preview*
   /**
    * Controls the density and padding of the table cells.
    * - Small: Compact view with minimal padding
    * - Middle: Standard padding and spacing
    * - Large: Relaxed view with more padding
    */
   export enum TableSize {
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0d33d457-197a-4cf8-9cd7-d29b8bca79a5/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0d33d457-197a-4cf8-9cd7-d29b8bca79a5?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0d33d457-197a-4cf8-9cd7-d29b8bca79a5?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0d33d457-197a-4cf8-9cd7-d29b8bca79a5?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0d33d457-197a-4cf8-9cd7-d29b8bca79a5)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:01e7d536-94da-4e72-bcdf-acf9843aa88f -->
   
   
   [](01e7d536-94da-4e72-bcdf-acf9843aa88f)



##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -438,11 +438,22 @@ function ListView<T extends object = any>({
               getTableBodyProps={getTableBodyProps}
               prepareRow={prepareRow}
               headerGroups={headerGroups}
+              setSortBy={setSortBy}
               rows={rows}
               columns={columns}
               loading={loading}
               highlightRowId={highlightRowId}
               columnsForWrapText={columnsForWrapText}
+              bulkSelectEnabled={bulkSelectEnabled}
+              selectedFlatRows={selectedFlatRows}
+              toggleRowSelected={(rowId, value) => {
+                const row = rows.find(r => r.id === rowId);
+                if (row) {
+                  prepareRow(row);
+                  row.toggleRowSelected(value);
+                }
+              }}

Review Comment:
   ### Missing documentation for complex row selection logic <sub>![category 
Documentation](https://img.shields.io/badge/Documentation-7c3aed)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   New prop `toggleRowSelected` being passed to TableCollection lacks 
documentation about its purpose and usage.
   
   ###### Why this matters
   Other developers may struggle to understand why the `prepareRow` call is 
necessary before toggling the row selection, potentially leading to bugs if 
this critical step is missed when modifying the code.
   
   ###### Suggested change ∙ *Feature Preview*
   Update the ListViewProps interface to include:
   ```typescript
   /**
    * Toggles row selection state. Handles necessary row preparation before 
selection.
    * @param rowId - The id of the row to toggle
    * @param value - The new selection state
    */
   toggleRowSelected?: (rowId: string | number, value: boolean) => void;
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a8bf466-410f-4ef5-a81b-b311c3e1cf9b/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a8bf466-410f-4ef5-a81b-b311c3e1cf9b?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a8bf466-410f-4ef5-a81b-b311c3e1cf9b?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a8bf466-410f-4ef5-a81b-b311c3e1cf9b?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a8bf466-410f-4ef5-a81b-b311c3e1cf9b)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:d5a44842-479a-4612-b438-8a79c35df39f -->
   
   
   [](d5a44842-479a-4612-b438-8a79c35df39f)



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

Reply via email to