michael-s-molina commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3961276619
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:
##########
@@ -55,29 +55,75 @@ const getCellStyle = (params: CellStyleParams) => {
let backgroundColor;
let color;
if (hasColumnColorFormatters) {
- columnColorFormatters!
- .filter(formatter => {
- const colTitle = formatter?.column?.includes('Main')
- ? formatter?.column?.replace('Main', '').trim()
- : formatter?.column;
- return colTitle === colDef.field;
- })
- .forEach(formatter => {
- const formatterResult =
- value || value === 0 ? formatter.getColorFromValue(value) : false;
- if (formatterResult) {
- if (
- formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR ||
- formatter.toTextColor
- ) {
- color = formatterResult;
- } else if (
- formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR
- ) {
- backgroundColor = formatterResult;
- }
+ const applyFormatter = (
+ formatter: ColorFormatters[number],
+ valueToFormat: typeof value,
+ ) => {
+ const formatterResult =
+ valueToFormat || valueToFormat === 0
+ ? formatter.getColorFromValue(valueToFormat)
+ : false;
+ if (formatterResult) {
+ if (
+ formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR ||
+ formatter.toTextColor
+ ) {
+ color = formatterResult;
+ } else if (
+ formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR
+ ) {
+ backgroundColor = formatterResult;
}
- });
+ }
+ };
+
+ // formatter.column can be a legacy display label ("Main colname") for
+ // time-comparison columns rather than the row's actual data key, so
+ // resolve it to the real field id before using it to read row values.
+ const resolveColumnKey = (columnKey: string) =>
+ columnKey.startsWith('Main ')
+ ? columnKey.slice('Main '.length)
+ : columnKey;
+
+ // Formatters with no formatting target color their own source column,
+ // keyed off this cell's own value.
+ columnColorFormatters!
+ .filter(
+ formatter =>
+ !formatter.columnFormatting &&
+ resolveColumnKey(formatter.column) === colDef.field,
+ )
+ .forEach(formatter => applyFormatter(formatter, value));
+
+ // Formatters with a real target column color that target column,
+ // keyed off the value in the formatter's own (source) column.
+ columnColorFormatters!
+ .filter(
+ formatter =>
+ formatter.columnFormatting &&
+ formatter.columnFormatting !== ObjectFormattingEnum.ENTIRE_ROW &&
+ resolveColumnKey(formatter.columnFormatting) === colDef.field,
+ )
+ .forEach(formatter =>
+ applyFormatter(
+ formatter,
+ node?.data?.[resolveColumnKey(formatter.column)],
+ ),
+ );
+
+ // Entire-row formatters apply to every cell in the row, keyed off the
+ // value in the formatter's own column rather than this cell's column.
+ columnColorFormatters!
+ .filter(
+ formatter =>
+ formatter.columnFormatting === ObjectFormattingEnum.ENTIRE_ROW,
+ )
Review Comment:
Fixed: `getCellStyle`'s entire-row branch now also matches
`formatter.toAllRow`, not just `columnFormatting === ENTIRE_ROW`, and the
own-column branch excludes `toAllRow` formatters to avoid double-applying them.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -368,75 +453,203 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
[emitCrossFilters, setDataMask, timeGrain, timestampFormatter],
);
+ const drillColumns = isUsingTimeComparison
+ ? (filteredColumns as InputColumn[])
+ : (columns as InputColumn[]);
+
+ const handleContextMenu = useCallback(
+ (event: CellContextMenuEvent) => {
+ if (!onContextMenu || isRawRecords || !event.column || !event.data) {
+ return;
+ }
+ const nativeEvent = event.event as MouseEvent | null | undefined;
+ if (!nativeEvent) return;
+ nativeEvent.preventDefault();
+ nativeEvent.stopPropagation();
+
+ const rowData = event.data as Record<string, DataRecordValue>;
+ const key = event.column.getColId();
+ const cellValue = event.value as DataRecordValue;
+ const colDef = event.column.getColDef();
+ const isMetric = Boolean(
+ colDef.context?.isMetric || colDef.context?.isPercentMetric,
+ );
+
+ const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
+ drillColumns.forEach(col => {
+ if (col.isMetric || col.isPercentMetric) return;
+ const dataRecordValue = rowData[col.key];
+
+ if (
+ dataRecordValue == null ||
+ (dataRecordValue instanceof DateWithFormatter &&
+ dataRecordValue.input == null)
+ ) {
Review Comment:
Fixed: both the drillToDetail null check and the drillBy `isCellValueNull`
check (line 536) now use `isEmptyDateInput` instead of `== null`.
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -654,3 +656,258 @@ def process(base_query_object: dict[str, Any]) ->
list[dict[str, Any]]:
return [result]
return build_query_context(self.data, process)
+
+
+def _get_table_chart_time_offsets(form_data: dict[str, Any]) -> list[Any]:
+ """
+ Resolve time_compare into the list of shifts buildQuery.ts sends as
+ time_offsets. table charts use a single-select time_compare control
+ whose choices include the special 'custom'/'inherit' shifts, which
+ resolve to start_date_offset/'inherit' rather than being used verbatim.
+ """
+ time_compare_shifts = ensure_is_array(form_data.get("time_compare"))
+ non_custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift not in ("custom",
"inherit")
+ ]
+ custom_or_inherit_shifts = [
Review Comment:
Fixed — same change as the Aug 12 comment on this line:
`_get_table_chart_time_offsets` now takes `base_query_object` and gates on
`is_time_comparison(...)`.
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -654,3 +656,258 @@ def process(base_query_object: dict[str, Any]) ->
list[dict[str, Any]]:
return [result]
return build_query_context(self.data, process)
+
+
+def _get_table_chart_time_offsets(form_data: dict[str, Any]) -> list[Any]:
+ """
+ Resolve time_compare into the list of shifts buildQuery.ts sends as
+ time_offsets. table charts use a single-select time_compare control
+ whose choices include the special 'custom'/'inherit' shifts, which
+ resolve to start_date_offset/'inherit' rather than being used verbatim.
+ """
+ time_compare_shifts = ensure_is_array(form_data.get("time_compare"))
+ non_custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift not in ("custom",
"inherit")
+ ]
+ custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift in ("custom",
"inherit")
+ ]
+
+ time_offsets: list[Any] = list(non_custom_or_inherit_shifts)
+ if "custom" in custom_or_inherit_shifts:
+ time_offsets.append(form_data.get("start_date_offset"))
+ if "inherit" in custom_or_inherit_shifts:
+ time_offsets.append("inherit")
+
+ # Dashboard filter override - allows dashboard-level time shifts to
+ # OVERRIDE chart-level time shift settings, mirroring buildQuery.ts.
+ extra_form_data_time_compare = (form_data.get("extra_form_data") or
{}).get(
+ "time_compare"
+ )
+ if extra_form_data_time_compare:
+ # extra_form_data.time_compare is typed as a single string on the
+ # frontend, but self.data comes from deserialized JSON with no
+ # runtime type guarantee — normalize defensively so an already-list
+ # value doesn't get double-nested into [[...]].
+ time_offsets = list(ensure_is_array(extra_form_data_time_compare))
+ return time_offsets
+
+
+def _reorder_table_chart_temporal_column(
+ columns: list[Any],
+ time_grain_sqla: Any,
+ temporal_columns_lookup: dict[str, Any],
+) -> list[Any]:
+ """
+ Move the first physical column with a temporal_columns_lookup entry to
+ the front of the columns list as a BASE_AXIS adhoc column, mirroring
+ buildQuery.ts's temporal-column handling in aggregate mode.
+ """
+ temporal_column = None
+ filtered_columns = []
+ for col in columns:
+ should_be_temporal = (
+ is_physical_column(col)
+ and time_grain_sqla
+ and temporal_columns_lookup.get(col)
+ )
+ if should_be_temporal and temporal_column is None:
+ temporal_column = {
+ "timeGrain": time_grain_sqla,
+ "columnType": "BASE_AXIS",
+ "sqlExpression": col,
+ "label": col,
+ "expressionType": "SQL",
+ }
+ else:
+ filtered_columns.append(col)
+ return [temporal_column] + filtered_columns if temporal_column else
filtered_columns
+
+
+class MigrateTableChart(MigrateViz):
+ source_viz_type = "table"
+ target_viz_type = "ag-grid-table"
+ remove_keys = {"allow_rearrange_columns", "allow_render_html"}
Review Comment:
Fixed: implemented `allow_rearrange_columns`/`allow_render_html` in v2 (new
control-panel checkboxes wired into `transformProps.ts`/`useColDefs.ts`,
defaulting to the pre-existing always-on behavior when absent) rather than
silently changing migrated charts.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/index.ts:
##########
@@ -46,6 +46,7 @@ const metadata = new ChartMetadata({
Behavior.InteractiveChart,
Behavior.DrillToDetail,
Behavior.DrillBy,
+ 'EXPORT_CURRENT_VIEW' as Behavior,
Review Comment:
Fixed: the export menu now checks for the presence of `clientView`/`columns`
rather than `rows.length`, and `downloadClientCSV`/`JSON`/`XLSX` all support an
empty-but-present snapshot, producing a header-only file instead of falling
back to an unfiltered backend export.
--
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]
