Copilot commented on code in PR #36050:
URL: https://github.com/apache/superset/pull/36050#discussion_r2519355681
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -470,6 +644,9 @@ export class TableRenderer extends Component {
namesMapping,
allowRenderHtml,
)}
+ <span role="columnheader button" tabIndex={0}>
+ {visibleSortIcon && getSortIcon(i)}
+ </span>
Review Comment:
The `role` attribute has an invalid value. ARIA roles cannot have multiple
space-separated values like `\"columnheader button\"`. The span should use
`role=\"button\"` since it's an interactive element for sorting. The parent
`<th>` element already has `role=\"columnheader button\"` which also needs to
be corrected to just `role=\"columnheader\"`.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -348,6 +420,84 @@ export class TableRenderer extends Component {
return spans;
}
+ calculateGroups(pivotData, visibleColName) {
+ const groups = {};
+ const rows = pivotData.rowKeys;
+ rows.forEach(rowKey => {
+ const aggValue = pivotData.getAggregator(rowKey, visibleColName).value();
+ let current = groups;
+ let sumGroup = 0;
+ rowKey.forEach(key => {
+ if (!current[key]) {
+ current[key] = { currentVal: 0 };
+ }
+ sumGroup += aggValue;
+ current[key].currentVal = sumGroup;
+ current = current[key];
+ });
+ });
+ return groups;
+ }
Review Comment:
The `sumGroup` variable is being accumulated incorrectly. It's initialized
to 0 for each `rowKey` and then adds `aggValue` for every nested key in the
hierarchy. This means if a rowKey has 3 levels, the aggValue gets multiplied by
3. The `sumGroup` should either be set to `aggValue` directly (not
accumulated), or the accumulation logic needs to be reconsidered based on the
actual requirements for hierarchical aggregation.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -470,6 +644,9 @@ export class TableRenderer extends Component {
namesMapping,
allowRenderHtml,
)}
+ <span role="columnheader button" tabIndex={0}>
Review Comment:
The span has `tabIndex={0}` making it keyboard-focusable, but there's no
`onKeyDown` handler to trigger sorting via keyboard. Users navigating with
keyboard won't be able to activate the sort functionality. Add an `onKeyDown`
handler that triggers the sort when Enter or Space is pressed.
```suggestion
<span
role="columnheader button"
tabIndex={0}
onKeyDown={e => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
this.clickHeaderHandler(
pivotData,
colKey,
this.props.cols,
attrIdx,
this.props.tableOptions.clickColumnHeaderCallback,
)(e);
}
}}
>
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -71,15 +74,84 @@ function displayHeaderCell(
);
}
+function sortHierarchicalObject(obj, objSort, rowPartialOnTop) {
+ const sortedKeys = Object.keys(obj).sort((a, b) => {
+ const valA = obj[a].currentVal || 0;
+ const valB = obj[b].currentVal || 0;
+ if (rowPartialOnTop) {
+ if (obj[a].currentVal !== undefined && obj[b].currentVal === undefined) {
+ return -1;
+ }
+ if (obj[b].currentVal !== undefined && obj[a].currentVal === undefined) {
+ return 1;
+ }
+ }
+ return objSort === 'asc' ? valA - valB : valB - valA;
+ });
+ return sortedKeys.reduce((acc, key) => {
+ acc[key] =
+ typeof obj[key] === 'object' && !Array.isArray(obj[key])
+ ? sortHierarchicalObject(obj[key], objSort, rowPartialOnTop)
+ : obj[key];
+ return acc;
+ }, {});
+}
+
+function convertToArray(
+ obj,
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ parentKeys = [],
+ result = [],
+ flag = false,
+) {
+ let updatedFlag = flag;
+ Object.keys(obj).forEach(key => {
+ if (key === 'currentVal') {
+ return;
+ }
+ if (rowEnabled && rowPartialOnTop && parentKeys.length < maxRowIndex - 1) {
+ result.push(parentKeys.length > 0 ? [...parentKeys, key] : [key]);
+ updatedFlag = true;
+ }
+ if (typeof obj[key] === 'object' && !Array.isArray(obj[key])) {
+ parentKeys.push(key);
+ convertToArray(
+ obj[key],
+ rowEnabled,
+ rowPartialOnTop,
+ maxRowIndex,
+ parentKeys,
+ result,
+ );
+ parentKeys.pop();
+ }
+ if (
+ parentKeys.length >= maxRowIndex - 1 ||
+ (rowEnabled && !rowPartialOnTop)
+ ) {
+ if (!updatedFlag) {
+ result.push(parentKeys.length > 0 ? [...parentKeys, key] : [key]);
+ return;
+ }
+ }
+ if (parentKeys.length === 0 && maxRowIndex === 1) {
+ result.push([key]);
+ }
+ });
+ return result;
+}
Review Comment:
The `updatedFlag` variable is set to `true` on line 116 but is never reset
to `false` when processing subsequent siblings. This means once one sibling
sets the flag, all following siblings at the same level will see
`updatedFlag=true`, potentially causing incorrect behavior in the conditional
on line 134. The flag should be reset at the appropriate scope or the logic
should be restructured to properly handle sibling iteration.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -433,11 +583,35 @@ export class TableRenderer extends Component {
) {
colLabelClass += ' active';
}
+ const { maxRowVisible: maxRowIndex, maxColVisible } = pivotSettings;
+ const visibleSortIcon = maxColVisible - 1 === attrIdx;
const rowSpan = 1 + (attrIdx === colAttrs.length - 1 ? rowIncrSpan :
0);
const flatColKey = flatKey(colKey.slice(0, attrIdx + 1));
const onArrowClick = needToggle ? this.toggleColKey(flatColKey) : null;
+ const getSortIcon = key => {
+ const { activeSortColumn, sortingOrder } = this.state;
+
+ if (activeSortColumn !== key) {
+ return (
+ <FaSort
+ onClick={() =>
+ this.sortData(key, visibleColKeys, pivotData, maxRowIndex)
+ }
+ />
+ );
+ }
+
+ const SortIcon = sortingOrder[key] === 'asc' ? FaSortAsc :
FaSortDesc;
+ return (
+ <SortIcon
+ onClick={() =>
+ this.sortData(key, visibleColKeys, pivotData, maxRowIndex)
+ }
+ />
+ );
+ };
Review Comment:
The `onClick` handler with identical parameters is duplicated in both
branches. Extract it to a common variable or function to reduce duplication and
improve maintainability. For example: `const handleClick = () =>
this.sortData(key, visibleColKeys, pivotData, maxRowIndex);` and then use
`onClick={handleClick}` in both icon components.
--
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]
