sadpandajoe commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4129750297
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1166,21 +1195,15 @@ export function TableRenderer(props:
TableRendererProps) {
</th>
);
})}
- <th
- className="pvtTotalLabel"
- key="padding"
- onClick={clickHeaderHandler(
- pivotData,
- [],
- rows,
- 0,
- tableOptions.clickRowHeaderCallback,
- false,
- true,
- )}
- >
- {settingsColAttrs.length === 0 ? t('Total') : null}
- </th>
+ {mergeTotalLabel ? null : (
Review Comment:
Now that the separate `.pvtTotalLabel` cell only renders when
`mergeTotalLabel` is false, the existing CSS rule `table.pvtTable thead
tr:last-of-type:not(:only-child) th.pvtAxisLabel + .pvtTotalLabel` (Styles.ts)
can no longer match anything: when column attributes exist, this cell is gone
entirely; when they don't, the row-header row is the `thead`'s only child,
which fails `:not(:only-child)`. Could that CSS rule be removed as dead code in
this PR?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -911,8 +901,21 @@ export function TableRenderer(props: TableRendererProps) {
subArrow =
attrIdx + 1 < maxColVisible! ? arrowExpanded : arrowCollapsed;
}
+ // With row dimensions, the corner block above the frozen row labels
+ // is rowAttrs.length + 1 columns wide: the row-attribute columns plus
+ // the padding column that renderTableRow folds into the last row
+ // label. Span the column-attribute name across that whole block so a
+ // single sticky cell freezes it, rather than a rowAttrs-wide spacer
+ // that leaves the name's own column scrolling through the corner.
+ const hasRowAttrs = settingsRowAttrs.length !== 0;
const attrNameCell = (
- <th key="label" className="pvtAxisLabel">
+ <th
+ key="label"
+ className={
+ hasRowAttrs ? 'pvtAxisLabel pvtCornerLabel' : 'pvtAxisLabel'
+ }
+ colSpan={hasRowAttrs ? settingsRowAttrs.length + 1 : undefined}
Review Comment:
The removed spacer cell was `aria-hidden`, so it never participated in
header/cell associations. This replacement `attrNameCell` (and the merged
row-attribute cell at line 1180) is a visible, named `<th>` that now spans the
row-label columns too. For a pivot with row dimension "Country" and column
dimension "Year", a screen reader can associate "Year" as a header for cells in
the Country column that it previously had no header relationship with. Should
these merged cells carry `scope`/`headers` attributes (or stay `aria-hidden`
for the spanned-but-unrelated portion) so the header association only covers
the column(s) it actually names?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +157,27 @@ export const Styles = styled.div<{ isDashboardEditMode:
boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
Review Comment:
With more than one row dimension (e.g. `country` and `city`), every
`.pvtRowLabel` cell and every row-header corner cell (`Styles.ts:55`) freezes
at `left: 0`, so after horizontal scrolling both frozen columns stack on the
same edge and the later one visually covers the earlier one. The PR description
scopes this to single-level row headers, but nothing in the renderer or CSS
actually gates multi-row-dimension pivots out of the new sticky behavior — they
just render broken. Should this be limited to the single-row-dimension case
(e.g. skip `left: 0` when there is more than one row attribute) until the
per-column offset fast-follow lands?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1137,12 +1140,29 @@ export function TableRenderer(props:
TableRendererProps) {
namesMapping,
allowRenderHtml: settingsAllowRenderHtml,
} = settings;
+ // When column attributes are present, renderTableRow gives the last
+ // row-label cell in the body an extra colSpan to absorb the corner
Review Comment:
This comment says `renderTableRow` "folds" the trailing placeholder column
into the last row-label cell via colSpan, but for a subtotal row
(`rowKey.length < settingsRowAttrs.length`) `renderTableRow` instead renders a
separate `attrValuePaddingCell` (`pvtRowLabel pvtSubtotalLabel`) rather than
extending the last label cell's colSpan. Could the comment be corrected to
describe the subtotal-row case accurately, so a future reader doesn't assume
the corner block's colSpan always matches a single extended body cell?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1154,7 +1174,16 @@ export function TableRenderer(props: TableRendererProps)
{
i + 1 < maxRowVisible! ? arrowExpanded : arrowCollapsed;
}
return (
- <th className="pvtAxisLabel" key={`rowAttr-${i}`}>
+ <th
+ className="pvtAxisLabel"
+ key={`rowAttr-${i}`}
+ colSpan={isLastRowAttr && mergeTotalLabel ? 2 : undefined}
Review Comment:
Before this change, the grand-total `clickHeaderHandler(...,
isGrandTotal=true)` only fired from the separate blank `pvtTotalLabel` cell.
Now that handler is also wired to `onClick` on the last row-attribute name
`<th>` (e.g. "city") whenever `mergeTotalLabel` is true, so clicking that label
text invokes the same grand-total callback as clicking the old blank spacer.
Superset's built-in `clickRowHeaderCallback` ignores grand totals, but a custom
callback consuming this contract would now see clicks on the row-attribute name
reported as grand-total clicks. Was this conflation intentional, or should the
row-attribute name keep its own click behavior separate from the grand-total
handler?
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx:
##########
@@ -0,0 +1,201 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import '@testing-library/jest-dom';
+import { render } from '@testing-library/react';
+import { supersetTheme } from '@apache-superset/core/theme';
+import PivotTableChart from '../../src/PivotTableChart';
+import transformProps from '../../src/plugin/transformProps';
+import testData from '../testData';
+import { ProviderWrapper } from '../testHelpers';
+
+test('sticky-positions the row-label column and its corner header cell(s) so
they stay visible while scrolling', () => {
+ const transformedProps = {
+ ...transformProps(testData.withoutColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const rowLabelCell = container.querySelector('tbody th.pvtRowLabel');
+ expect(rowLabelCell).toBeInTheDocument();
+ const rowLabelStyle = getComputedStyle(rowLabelCell as Element);
+ expect(rowLabelStyle.position).toBe('sticky');
+ expect(rowLabelStyle.left).toBe('0px');
+
+ // The corner cells above the frozen row-label column (the column
+ // attribute name cell in each column-header row and the row-attribute
+ // name cell in the row-header row) must stick on both axes and paint
+ // over the column labels that scroll underneath them.
+ const cornerCells = [
Review Comment:
`cornerCells.length` is satisfied as soon as either `thead
th.pvtCornerLabel` or `thead tr.pvtRowHeaderRow th.pvtAxisLabel` matches, so if
`pvtCornerLabel` were dropped from the column-attribute name cell, the
column-header corner would scroll away with no sticky/z-index protection, but
this test would still pass on the row-header cells alone. Separately,
`getComputedStyle(...).zIndex` on an element with no declared `z-index`
resolves to `''` in jsdom, so `Number(...)` is `0` — the `> 0` comparisons here
(and in the totals-row test) would still pass even if the `z-index` rule on
`.pvtRowLabel`/`.pvtCornerLabel` were removed entirely. Could each corner-cell
group be asserted non-empty separately, and the z-index assertions check the
declared value rather than just `> 0`?
--
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]
