Re: [PR] feat(table v2): agGridTableChart add row numer column [superset]

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

SBIN2010 commented on PR #39284:
URL: https://github.com/apache/superset/pull/39284#issuecomment-4468427979

   Adjustments have been made
   
   > * **`currentmaxPageNumber` is a page count or digit count, not pixels. This might make the column incorrectly sized. Maybe it should be `30 + rowIndexLength * 6` (same formula as `width`) or simply removed.
   
   `currentmaxPageNumber` - removed
   
   > * **Pinned totals row gets a row number.** When `showTotals` is on, the `valueGetter` is called for the pinned bottom row too, so it will display a fake row number. Add an early return: `if (params.node?.rowPinned != null) return '';`
   
   `if (params.node?.rowPinned != null) return '';` - Added
   
   > * **`pageSize` can be `0` when `serverPageLength` is unset.** The nullish-coalescing chain `?? serverPageLength ?? data.length` will accept `0` as a valid page size, breaking offset math. Guard against non-positive values.
   
   removed
   
   > * **`PIVOT_COL_ID = '-1'` may be a misleading or unsafe name/value.** The name implies pivot functionality, and `'-1'` could collide with a real column key in saved chart state. Maybe renaming to `ROW_NUMBER_COL_ID` with a value like `'__row_number__'` would help.
   
   Name PIVOT_COL_ID replaced with `ROW_NUMBER_COL_ID` value replaced with `'__row_number__'`
   
   > * **Cross-filter risk.** If `emitCrossFilters` is on, clicking a cell in the row number column will emit a cross-filter payload for field `'-1'`, which maps to no real dataset column. The column should be excluded or its click handler short-circuited when cross-filters are enabled.
   
   Added - !emitCrossFilters
   display on the dashboard when cross-filtering is disabled
   
   > * **`context: { isMetric: true }` is incorrect.** This may cause the row number column to be treated as a metric by other pipeline stages (color formatting, cross-filter handlers). Maybe a dedicated flag like `{ isRowNumber: true }` would help.
   
   `context: { isMetric: true }` - removed
   
   > * `headerName: '№'` should go through `t()` for i18n consistency with the rest of the plugin.
   
   `t()` - Added
   
   


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