You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/09/05 02:07:23 UTC
[PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
rusackas opened a new pull request, #43917:
URL: https://github.com/apache/superset/pull/43917
### SUMMARY
Column headers (`<thead>`) and row totals (`.pvtRowTotals`) already stick via CSS, but the row-label column and the corner cell(s) above it did not, so on wide/tall pivot tables they scrolled out of view while the header and totals row stayed put. This adds `position: sticky; left: 0` to `.pvtRowLabel` cells, plus a combined top+left sticky rule (with a higher z-index so it wins over the row-label column at the intersection) for the corner cell(s): the placeholder cell spanning the column-attribute rows, and the row-attribute name cell(s) in the final header row. Both are gated by the existing `isDashboardEditMode` flag, same as the pre-existing sticky rules.
This is scoped to single-level row headers (a fixed `left: 0` offset). Multi-level row headers (more than one row dimension) need a per-column inline `left` style computed in `TableRenderers.tsx`, which is a reasonable fast-follow rather than something that should block this fix, per the discussion on #43352.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (CSS-only change to an existing scroll behavior; see testing instructions to verify visually).
### TESTING INSTRUCTIONS
1. Added `plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx`, which renders a pivot table and asserts (via `getComputedStyle`) that `.pvtRowLabel` and the corner header cell(s) are `position: sticky` with the expected offsets and z-index ordering, and that this is suppressed in dashboard edit mode. This test fails against the pre-fix `Styles.ts` and passes with the fix.
2. Manually: create a Pivot Table chart with enough rows/columns to require scrolling in both directions, and confirm the row-label column and the corner cell stay pinned to the left/top while scrolling.
```
npm run test -- plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx
```
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #43352
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5902413266
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5558081340
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4137724743
##########
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
Review Comment:
This says folding the corner cell in lets "a single sticky cell freeze it" — but `canFreezeRowLabels = rowAttrs.length <= 1` (added later in this same PR) means that's only true for a single row dimension; with more than one, this cell falls back to `position: inherit`, not sticky. The "frozen corner block" phrasing at line 1150 has the same gap. Could both comments be qualified for the multi-row-dimension case?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1154,7 +1177,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}
+ onClick={
+ isLastRowAttr && mergeTotalLabel
+ ? totalLabelClickHandler
Review Comment:
With one row attribute and one column attribute, clicking this cell is meant to invoke `clickRowHeaderCallback` as a grand-total click, absorbing the removed blank `pvtTotalLabel` cell's click contract — but no test clicks it. `Styles.test.tsx` only asserts this cell's span and CSS, and neither `TableRenderer.test.tsx` nor `tableRenders.test.tsx` simulate a click on it. Could a test click this cell and assert `clickRowHeaderCallback` fires with `isGrandTotal=true`?
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx:
##########
@@ -0,0 +1,246 @@
+/**
+ * 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. Asserted as two
+ // separate non-empty groups -- rather than one merged list -- so that if
+ // either selector stops matching (e.g. `pvtCornerLabel` is dropped from
+ // the column-attribute name cell), that group's absence fails the test
+ // instead of silently leaving only the other group's cells checked.
+ const columnCornerCells = container.querySelectorAll(
+ 'thead th.pvtCornerLabel',
+ );
+ const rowHeaderCornerCells = container.querySelectorAll(
+ 'thead tr.pvtRowHeaderRow th.pvtAxisLabel',
+ );
+ expect(columnCornerCells.length).toBeGreaterThan(0);
+ expect(rowHeaderCornerCells.length).toBeGreaterThan(0);
+ const cornerCells = [...columnCornerCells, ...rowHeaderCornerCells];
+ // The frozen block in each column-header row has to cover the same
+ // columns as the frozen row label below it (its own column plus the
+ // padding column), otherwise the uncovered strip shows column labels
+ // scrolling through the corner.
+ const rowLabelSpan = (rowLabelCell as HTMLTableCellElement).colSpan;
+ expect(rowLabelSpan).toBe(2);
+ columnCornerCells.forEach(cell => {
+ expect((cell as HTMLTableCellElement).colSpan).toBe(rowLabelSpan);
+ });
+ cornerCells.forEach(cell => {
+ const style = getComputedStyle(cell);
+ expect(style.position).toBe('sticky');
+ expect(style.top).toBe('0px');
+ expect(style.left).toBe('0px');
+ // Assert the declared value, not just a positive number: with no
+ // z-index rule applied, jsdom's getComputedStyle resolves to '' and
+ // Number('') is 0, so a bare `> 0` check would still pass.
+ expect(style.zIndex).toBe('1');
+ });
+
+ // The sticky thead is its own stacking context, so the corner cell's
+ // z-index can't outrank the row labels by itself. The thead as a whole
+ // has to sit above the frozen row-label column.
+ const thead = container.querySelector('thead');
+ expect(thead).toBeInTheDocument();
+ expect(getComputedStyle(thead as Element).zIndex).toBe('2');
+ expect(rowLabelStyle.zIndex).toBe('1');
+});
+
+test('keeps the sticky totals row above the frozen row-label column', () => {
+ const transformedProps = {
+ ...transformProps(testData.withColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const rowLabelCell = container.querySelector('tbody th.pvtRowLabel');
+ const totalsRow = container.querySelector('tbody tr.pvtRowTotals');
+ expect(rowLabelCell).toBeInTheDocument();
+ expect(totalsRow).toBeInTheDocument();
+ expect(getComputedStyle(totalsRow as Element).zIndex).toBe('2');
+ expect(getComputedStyle(rowLabelCell as Element).zIndex).toBe('1');
+
+ // The totals row's leading label freezes at the left edge alongside the
+ // body row labels, and sits above the totals values in its own row.
+ const totalsLabel = totalsRow?.querySelector('th.pvtRowTotalLabel');
+ expect(totalsLabel).toBeInTheDocument();
+ const totalsLabelStyle = getComputedStyle(totalsLabel as Element);
+ expect(totalsLabelStyle.position).toBe('sticky');
+ expect(totalsLabelStyle.left).toBe('0px');
+ // Assert the declared value rather than `> 0`: with no z-index rule
+ // applied, jsdom resolves getComputedStyle(...).zIndex to '', and
+ // Number('') is 0, so a bare positivity check would still pass.
+ expect(totalsLabelStyle.zIndex).toBe('1');
+ expect((totalsLabel as HTMLTableCellElement).colSpan).toBe(
+ (rowLabelCell as HTMLTableCellElement).colSpan,
+ );
+});
+
+test('lets the active (cross-filter) highlight win over the frozen row-label background', () => {
+ const transformedProps = {
+ ...transformProps(testData.withoutColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ // Computed colors come back normalized (rgb()), so run the theme tokens
+ // through the same normalization before comparing.
+ const normalizeColor = (color: string) => {
+ const probe = document.createElement('div');
+ probe.style.backgroundColor = color;
+ return probe.style.backgroundColor;
+ };
+
+ const rowLabelCell = container.querySelector('tbody th.pvtRowLabel');
+ expect(rowLabelCell).toBeInTheDocument();
+ expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe(
+ normalizeColor(supersetTheme.colorBgBase),
+ );
+
+ rowLabelCell?.classList.add('active');
+ expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe(
+ normalizeColor(supersetTheme.colorPrimaryBg),
+ );
+});
+
+test('does not freeze any header cell when the pivot has column dimensions but no row dimensions', () => {
+ const transformedProps = {
+ ...transformProps(testData.columnsOnly),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ // Without row dimensions there is no row-header row, so the leading
+ // cell of the last header row is the column attribute name. It must
+ // scroll with its column rather than being treated as a corner cell.
+ expect(container.querySelector('thead tr.pvtRowHeaderRow')).toBeNull();
+ const headerCells = container.querySelectorAll('thead th');
+ expect(headerCells.length).toBeGreaterThan(0);
+ headerCells.forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+});
+
+test('does not freeze the row-label column or its corner cell(s) with more than one row dimension', () => {
+ // With multiple row attributes, every row-label and corner cell would
+ // freeze at the same left: 0 edge and stack on top of one another, so
+ // freezing is scoped to the single-row-dimension case until a
+ // per-column offset fast-follow lands.
+ const transformedProps = {
+ ...transformProps(testData.groupedRowsWithoutColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const rowLabelCells = container.querySelectorAll('tbody th.pvtRowLabel');
+ expect(rowLabelCells.length).toBeGreaterThan(0);
+ rowLabelCells.forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+
+ const cornerCells = [
Review Comment:
This merges `columnCornerCells` (`thead th.pvtCornerLabel`) into the same array as the row-header cells before asserting non-sticky, but `testData.groupedRowsWithoutColTotals` has `groupbyColumns: []`, so there are zero `.pvtCornerLabel` elements here — the assertions below are satisfied entirely by the row-header cells, and the `canFreezeRowLabels` gate on the `.pvtCornerLabel` selector in Styles.ts is never exercised by this test. Could the two selector groups be asserted separately (as the sticky-positioning test above already does), or a fixture added with both multiple row dimensions and a column dimension?
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3941614990
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -39,6 +39,20 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
top: 0;
}
+ /* Corner cell(s) sitting above the frozen row-label column: the
+ * placeholder cell spanning the column-attribute rows, and the
+ * row-attribute name cell(s) in the final header row. Needs a
+ * higher z-index than .pvtRowLabel so it stays on top when both
+ * stick at the same scroll position. */
+ table.pvtTable thead tr:first-of-type th[aria-hidden='true'],
+ table.pvtTable thead tr:last-of-type th.pvtAxisLabel {
Review Comment:
When a Pivot Table has columns but no row dimensions, the row-header `<tr>` is absent, so this selector matches the column attribute name instead of a corner cell. That header then freezes at `left: 0` and covers column labels while scrolling horizontally. Should this be scoped to the row-header cells, and covered with a rows-empty fixture?
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4268757403
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +165,33 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
+ position: ${
+ isDashboardEditMode || !canFreezeRowLabels ? 'inherit' : 'sticky'
+ };
+ left: 0;
+ z-index: 1;
+ background-color: ${theme.colorBgBase};
+ /* Cap the frozen label so a long unbroken value can't grow wider
+ * than the scroll area and cover every metric cell. */
+ max-width: 50%;
Review Comment:
Measured in Chrome 146: inert. `max-width` does not apply to a `table-cell` under `table-layout: auto`, so this row label renders **441.6px with and without these two lines** (table 764px; 50% would be 382px) and a long unbroken value still covers the metric cells.
##########
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:
Measured rather than reasoned, since this turns on the real accessibility computation: I rendered the component at the merge base and at head, then ran axe-core 4.14.0's table header computation over both DOMs.
**Every data cell's header list is byte-identical before and after**, in all three fixtures.
With a single row dimension — the only configuration where this PR turns freezing on — the row-label `th`'s header set goes from `{spacer, Metric, city, country, empty pvtTotalLabel}` to `{Metric, city, country}`. `Metric` and `city` were *already* associated before the merge, because the row-label cell already carried `colSpan=2`; the merge only drops the two semantically empty cells. So in the shipping case the association set strictly shrinks.
The one real residue is at two or more row dimensions: the corner cell spans `rowAttrs.length + 1` = 3 columns, so the first-level row label newly picks up `Metric`/`quarter`. No data cell changes there either, and `canFreezeRowLabels` is false in that configuration, so the merge buys nothing there — gating the `colSpan` on `rowAttrs.length <= 1` alongside the CSS would drop it if you want it gone.
Net: the association you flagged does not reach any data cell, and it is not newly introduced in the single-row-dimension case.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4118835721
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -33,10 +33,30 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
line-height: 1.4;
}
+ /* The sticky thead and totals row each form their own stacking
+ * context, so a z-index on a cell inside them can't outrank the
+ * frozen row labels (z-index 1) in the body. The bands themselves
+ * carry the z-index that keeps them painting over row labels
+ * scrolling underneath. */
table thead {
background-color: ${theme.colorBgBase};
position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
top: 0;
+ z-index: 2;
+ }
+
+ /* Corner cell(s) sitting above the frozen row-label column: the
+ * placeholder cell spanning the column-attribute rows, and the
+ * row-attribute name cell(s) in the row-header row (which only
+ * renders when there are row dimensions). The z-index keeps them
+ * over the column labels scrolling underneath within the thead. */
+ table.pvtTable thead tr:first-of-type th[aria-hidden='true'],
Review Comment:
Verified this locally — the fix is real. `attrNameCell` now spans `rowAttrs.length + 1` columns (class `pvtCornerLabel`) in place of the old `rowAttrs`-wide spacer, so the frozen block in every thead row matches the merged row-label span below it. Test asserts the colSpan match directly. Ran the suite, 5/5 pass.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -55,6 +75,7 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr.pvtRowTotals {
Review Comment:
Also verified — `th.pvtRowTotalLabel` in `tr.pvtRowTotals` is sticky at left:0 with z-index 1 now, frozen with the body row labels while the totals values scroll underneath. Covered by the same test run, 5/5 pass.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4154904409
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx:
##########
@@ -0,0 +1,282 @@
+/**
+ * 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. Asserted as two
+ // separate non-empty groups -- rather than one merged list -- so that if
+ // either selector stops matching (e.g. `pvtCornerLabel` is dropped from
+ // the column-attribute name cell), that group's absence fails the test
+ // instead of silently leaving only the other group's cells checked.
+ const columnCornerCells = container.querySelectorAll(
+ 'thead th.pvtCornerLabel',
+ );
+ const rowHeaderCornerCells = container.querySelectorAll(
+ 'thead tr.pvtRowHeaderRow th.pvtAxisLabel',
+ );
+ expect(columnCornerCells.length).toBeGreaterThan(0);
+ expect(rowHeaderCornerCells.length).toBeGreaterThan(0);
+ const cornerCells = [...columnCornerCells, ...rowHeaderCornerCells];
+ // The frozen block in each column-header row has to cover the same
+ // columns as the frozen row label below it (its own column plus the
+ // padding column), otherwise the uncovered strip shows column labels
+ // scrolling through the corner.
+ const rowLabelSpan = (rowLabelCell as HTMLTableCellElement).colSpan;
+ expect(rowLabelSpan).toBe(2);
+ columnCornerCells.forEach(cell => {
+ expect((cell as HTMLTableCellElement).colSpan).toBe(rowLabelSpan);
Review Comment:
Added — the row-header corner cells now get the same colSpan assertion as the column-attribute corner cell.
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx:
##########
@@ -0,0 +1,282 @@
+/**
+ * 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. Asserted as two
+ // separate non-empty groups -- rather than one merged list -- so that if
+ // either selector stops matching (e.g. `pvtCornerLabel` is dropped from
+ // the column-attribute name cell), that group's absence fails the test
+ // instead of silently leaving only the other group's cells checked.
+ const columnCornerCells = container.querySelectorAll(
+ 'thead th.pvtCornerLabel',
+ );
+ const rowHeaderCornerCells = container.querySelectorAll(
+ 'thead tr.pvtRowHeaderRow th.pvtAxisLabel',
+ );
+ expect(columnCornerCells.length).toBeGreaterThan(0);
+ expect(rowHeaderCornerCells.length).toBeGreaterThan(0);
+ const cornerCells = [...columnCornerCells, ...rowHeaderCornerCells];
+ // The frozen block in each column-header row has to cover the same
+ // columns as the frozen row label below it (its own column plus the
+ // padding column), otherwise the uncovered strip shows column labels
+ // scrolling through the corner.
+ const rowLabelSpan = (rowLabelCell as HTMLTableCellElement).colSpan;
+ expect(rowLabelSpan).toBe(2);
+ columnCornerCells.forEach(cell => {
+ expect((cell as HTMLTableCellElement).colSpan).toBe(rowLabelSpan);
+ });
+ cornerCells.forEach(cell => {
+ const style = getComputedStyle(cell);
+ expect(style.position).toBe('sticky');
+ expect(style.top).toBe('0px');
+ expect(style.left).toBe('0px');
+ // Assert the declared value, not just a positive number: with no
+ // z-index rule applied, jsdom's getComputedStyle resolves to '' and
+ // Number('') is 0, so a bare `> 0` check would still pass.
+ expect(style.zIndex).toBe('1');
+ });
+
+ // The sticky thead is its own stacking context, so the corner cell's
+ // z-index can't outrank the row labels by itself. The thead as a whole
+ // has to sit above the frozen row-label column.
+ const thead = container.querySelector('thead');
+ expect(thead).toBeInTheDocument();
+ expect(getComputedStyle(thead as Element).zIndex).toBe('2');
+ expect(rowLabelStyle.zIndex).toBe('1');
+});
+
+test('keeps the sticky totals row above the frozen row-label column', () => {
+ const transformedProps = {
+ ...transformProps(testData.withColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const rowLabelCell = container.querySelector('tbody th.pvtRowLabel');
+ const totalsRow = container.querySelector('tbody tr.pvtRowTotals');
+ expect(rowLabelCell).toBeInTheDocument();
+ expect(totalsRow).toBeInTheDocument();
+ expect(getComputedStyle(totalsRow as Element).zIndex).toBe('2');
+ expect(getComputedStyle(rowLabelCell as Element).zIndex).toBe('1');
+
+ // The totals row's leading label freezes at the left edge alongside the
+ // body row labels, and sits above the totals values in its own row.
+ const totalsLabel = totalsRow?.querySelector('th.pvtRowTotalLabel');
+ expect(totalsLabel).toBeInTheDocument();
+ const totalsLabelStyle = getComputedStyle(totalsLabel as Element);
+ expect(totalsLabelStyle.position).toBe('sticky');
+ expect(totalsLabelStyle.left).toBe('0px');
+ // Assert the declared value rather than `> 0`: with no z-index rule
+ // applied, jsdom resolves getComputedStyle(...).zIndex to '', and
+ // Number('') is 0, so a bare positivity check would still pass.
+ expect(totalsLabelStyle.zIndex).toBe('1');
+ expect((totalsLabel as HTMLTableCellElement).colSpan).toBe(
+ (rowLabelCell as HTMLTableCellElement).colSpan,
+ );
+});
+
+test('lets the active (cross-filter) highlight win over the frozen row-label background', () => {
+ const transformedProps = {
+ ...transformProps(testData.withoutColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ // Computed colors come back normalized (rgb()), so run the theme tokens
+ // through the same normalization before comparing.
+ const normalizeColor = (color: string) => {
+ const probe = document.createElement('div');
+ probe.style.backgroundColor = color;
+ return probe.style.backgroundColor;
+ };
+
+ const rowLabelCell = container.querySelector('tbody th.pvtRowLabel');
+ expect(rowLabelCell).toBeInTheDocument();
+ expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe(
+ normalizeColor(supersetTheme.colorBgBase),
+ );
+
+ rowLabelCell?.classList.add('active');
+ expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe(
+ normalizeColor(supersetTheme.colorPrimaryBg),
+ );
+});
+
+test('does not freeze any header cell when the pivot has column dimensions but no row dimensions', () => {
+ const transformedProps = {
+ ...transformProps(testData.columnsOnly),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ // Without row dimensions there is no row-header row, so the leading
+ // cell of the last header row is the column attribute name. It must
+ // scroll with its column rather than being treated as a corner cell.
+ expect(container.querySelector('thead tr.pvtRowHeaderRow')).toBeNull();
+ const headerCells = container.querySelectorAll('thead th');
+ expect(headerCells.length).toBeGreaterThan(0);
+ headerCells.forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+});
+
+test('does not freeze the row-label column or its corner cell(s) with more than one row dimension', () => {
+ // With multiple row attributes, every row-label and corner cell would
+ // freeze at the same left: 0 edge and stack on top of one another, so
+ // freezing is scoped to the single-row-dimension case until a
+ // per-column offset fast-follow lands. Uses a fixture with both multiple
+ // row dimensions and a column dimension (unlike groupedRowsWithoutColTotals,
+ // which has no column dimension) so the pvtCornerLabel group below is
+ // actually non-empty and its non-sticky assertion is exercised, rather
+ // than passing vacuously.
+ const transformedProps = {
+ ...transformProps(testData.groupedRowsWithColumnDim),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const rowLabelCells = container.querySelectorAll('tbody th.pvtRowLabel');
+ expect(rowLabelCells.length).toBeGreaterThan(0);
+ rowLabelCells.forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+
+ // Asserted as two separate non-empty groups, mirroring the
+ // sticky-positioning test above, so that if either selector stops
+ // matching, that group's absence fails the test instead of silently
+ // leaving only the other group's cells checked.
+ const columnCornerCells = container.querySelectorAll(
+ 'thead th.pvtCornerLabel',
+ );
+ const rowHeaderCornerCells = container.querySelectorAll(
+ 'thead tr.pvtRowHeaderRow th.pvtAxisLabel',
+ );
+ expect(columnCornerCells.length).toBeGreaterThan(0);
+ expect(rowHeaderCornerCells.length).toBeGreaterThan(0);
+ [...columnCornerCells, ...rowHeaderCornerCells].forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+});
+
+test('does not leak sticky positioning onto the totals label from its row with more than one row dimension', () => {
+ // th.pvtRowTotalLabel's direct parent is tr.pvtRowTotals, which is
+ // unconditionally sticky (it freezes to the bottom edge regardless of
+ // row-dimension count). The non-sticky branch here must resolve to an
+ // explicit 'static' rather than 'inherit', or the label would pick up
+ // the row's own sticky position and stay pinned at left: 0 anyway.
+ const transformedProps = {
+ ...transformProps(testData.groupedRowsWithColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const totalsLabel = container.querySelector(
+ 'tbody tr.pvtRowTotals th.pvtRowTotalLabel',
+ );
+ expect(totalsLabel).toBeInTheDocument();
+ expect(getComputedStyle(totalsLabel as Element).position).toBe('static');
+});
+
+test('does not sticky-position the row-label column or corner cell(s) in dashboard edit mode', () => {
+ // TableRenderers detects dashboard edit mode by looking for this class
+ // on the document, rather than via a prop.
+ const editingMarker = document.createElement('div');
+ editingMarker.className = 'dashboard--editing';
+ document.body.appendChild(editingMarker);
+
+ try {
+ 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();
+ expect(getComputedStyle(rowLabelCell as Element).position).not.toBe(
Review Comment:
Done — switched this to the totals-enabled fixture so it also asserts both corner-cell groups and the totals label stay non-sticky in edit mode, not just the row label.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5617430747
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5548656914
<!-- Bito Reply -->
The issue is correct. When multiple row attributes are present, each `th.pvtRowLabel` cell needs a unique `left` offset corresponding to its column index to prevent them from overlapping at `left: 0`.
To resolve this, you can dynamically set the `left` property based on the column index. Since `react-pivottable` renders these labels in a table structure, you can use the `nth-child` selector or a data attribute if available. Given the current implementation, a CSS-only approach using `nth-child` is the most concise fix:
```typescript
// In Styles.ts, update the pvtRowLabel selector:
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
z-index: 1;
background-color: ${theme.colorBgBase};
}
/* Add specific offsets for nested row labels */
table.pvtTable tbody tr th.pvtRowLabel:nth-child(1) { left: 0; }
table.pvtTable tbody tr th.pvtRowLabel:nth-child(2) { left: 100px; } /* Adjust width as needed */
```
Would you like me to check the rest of the comments on this PR to see if there are other issues to address?
**superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts**
```
table.pvtTable tbody tr th.pvtRowLabel:nth-child(1) { left: 0; }
table.pvtTable tbody tr th.pvtRowLabel:nth-child(2) { left: 100px; }
```
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4140846522
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -63,6 +100,19 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
background-color: ${theme.colorBgBase};
}
+ /* The totals row's leading label freezes at the left edge like the
+ * body row labels above it. The z-index keeps it over the totals
+ * values scrolling underneath within the row's own stacking context.
+ * Scoped to the single-row-dimension case for the same reason as the
+ * corner cells above. */
+ table.pvtTable tbody tr.pvtRowTotals th.pvtRowTotalLabel {
+ position: ${
+ isDashboardEditMode || !canFreezeRowLabels ? 'inherit' : 'sticky'
Review Comment:
With multiple row dimensions, `inherit` takes the sticky position from `.pvtRowTotals`, so this totals label still remains pinned at `left: 0` while the other row labels scroll normally. Should this branch use a non-sticky position instead?
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5548632048
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5910635198
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5549945841
Pushed a fix for Bito's test-assertion catch: `getComputedStyle().position` resolves `inherit` to the parent's computed value in a real browser rather than returning it verbatim, so the dashboard-edit-mode test was only passing because jsdom returns the literal string. Now asserting `not.toBe('sticky')` instead, which is the actual intent.
On the nth-child/fixed-pixel-offset suggestion for multi-level row headers: that won't work since column widths vary by content, a hardcoded `100px` would only be right by coincidence. Same answer as the CodeAnt thread below, this is the single-row-attribute scoping call from the PR description, multi-level needs real per-column offsets computed from rendered widths, which I'm treating as a fast-follow.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3941607641
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +134,10 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
+ position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
+ left: 0;
+ z-index: 1;
+ background-color: ${theme.colorBgBase};
}
Review Comment:
This selector is more specific than both `table.pvtTable tr th.active` and `.hoverable:hover`, so its background wins even when a row label is active or hovered. That hides the existing cross-filter/hover indication on row headers. Should those state styles take precedence over the frozen-cell background?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -39,6 +39,20 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
top: 0;
}
+ /* Corner cell(s) sitting above the frozen row-label column: the
+ * placeholder cell spanning the column-attribute rows, and the
+ * row-attribute name cell(s) in the final header row. Needs a
+ * higher z-index than .pvtRowLabel so it stays on top when both
+ * stick at the same scroll position. */
+ table.pvtTable thead tr:first-of-type th[aria-hidden='true'],
+ table.pvtTable thead tr:last-of-type th.pvtAxisLabel {
+ position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
+ top: 0;
+ left: 0;
+ z-index: 2;
Review Comment:
The sticky `thead` is a separate stacking context, so this cell's `z-index: 2` cannot outrank the row labels' `z-index: 1`. After scrolling both axes, a row label can paint over the frozen corner header. Should the header/totals layer be given an explicit higher stacking order?
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5890601044
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4131756999
##########
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:
Intentional — that cell absorbs the old blank `pvtTotalLabel` spacer when merged, so its click contract comes along with it. `clickRowHeaderCallback` ignores grand totals so nothing changes for the built-in case, but yeah, a custom callback would now see a click on the label text reported as a grand-total click.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3939706940
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +134,10 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
+ position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
+ left: 0;
+ z-index: 1;
+ background-color: ${theme.colorBgBase};
}
Review Comment:
Accurate for multi-level row headers, but it's the scoping call from the PR description: fixed `left: 0` only works cleanly for a single row attribute. Multi-level would need a per-column offset computed from actual rendered widths in `TableRenderers.tsx`, which felt like more than this fix should carry. Tracking that as a fast-follow per the discussion on #43352.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3975988815
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -33,10 +33,30 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
line-height: 1.4;
}
+ /* The sticky thead and totals row each form their own stacking
+ * context, so a z-index on a cell inside them can't outrank the
+ * frozen row labels (z-index 1) in the body. The bands themselves
+ * carry the z-index that keeps them painting over row labels
+ * scrolling underneath. */
table thead {
background-color: ${theme.colorBgBase};
position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
top: 0;
+ z-index: 2;
+ }
+
+ /* Corner cell(s) sitting above the frozen row-label column: the
+ * placeholder cell spanning the column-attribute rows, and the
+ * row-attribute name cell(s) in the row-header row (which only
+ * renders when there are row dimensions). The z-index keeps them
+ * over the column labels scrolling underneath within the thead. */
+ table.pvtTable thead tr:first-of-type th[aria-hidden='true'],
Review Comment:
Good catch, fixed in d2487a7d8f1f939c51d03dcc8111076d62ffcee8: dropped the rowAttrs-wide spacer and gave the column-attribute name cell a colSpan of rowAttrs + 1 (tagged `pvtCornerLabel`, sticky at left 0), so the frozen block in every thead row spans the same columns as the merged row label below it. The test asserts the corner colSpan matches the row label.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -55,6 +75,7 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr.pvtRowTotals {
Review Comment:
Yep, it should. Fixed in d2487a7d8f1f939c51d03dcc8111076d62ffcee8: `th.pvtRowTotalLabel` is sticky at left 0 with z-index 1 inside the totals row's own stacking context, so it stays frozen with the body row labels and the totals values scroll under it.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4288811460
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +165,33 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
+ position: ${
+ isDashboardEditMode || !canFreezeRowLabels ? 'inherit' : 'sticky'
+ };
+ left: 0;
+ z-index: 1;
+ background-color: ${theme.colorBgBase};
+ /* Cap the frozen label so a long unbroken value can't grow wider
+ * than the scroll area and cover every metric cell. */
+ max-width: 50%;
Review Comment:
Confirmed, and tracked it down further: `max-width` alone did nothing because `overflow-wrap: anywhere` was already doing all of the real work (measured: no styles at all gives an unconstrained column, overflow-wrap alone already cuts it roughly in half, max-width on top changes nothing). Dropped the dead `max-width: 50%` and left `overflow-wrap: anywhere` as the actual mechanism, with a comment explaining why. Updated the jsdom test too, it was asserting the declared CSS value rather than anything jsdom can actually verify about real layout. Thanks for measuring instead of guessing here, caught something a static read never would have.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4252844486
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +165,29 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
+ position: ${
Review Comment:
Good catch @sadpandajoe. Capped the frozen row label at `max-width: 50%` with `overflow-wrap: anywhere`, so a long unbroken value wraps instead of covering the metric cells. Not verified in a real browser yet, only asserted in the jsdom test.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5549973683
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | f776339ab661f527a568fda13b55bd27fac0be0b |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a9bb517ac51050008eb3161 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43917--superset-docs-preview.netlify.app](https://deploy-preview-43917--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcxNDgIhDEDhu3RtI4iMPzeYY8DQiSRDaSgkGuPdZfcWL98XpNUiHZ4AJ4gt8Paavef3OasOwqtz_oK5yEGFuGOqOlBxDpgZdQg1pY57q9yJp5FIjvpZ01SW8IjR21vYvDXeGHOn6Oxi4fcH) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43917--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4139053254
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1154,7 +1177,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}
+ onClick={
+ isLastRowAttr && mergeTotalLabel
+ ? totalLabelClickHandler
Review Comment:
Added a click test in `tableRenders.test.tsx` asserting `clickRowHeaderCallback` fires with `isGrandTotal=true` when that cell is clicked.
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx:
##########
@@ -0,0 +1,246 @@
+/**
+ * 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. Asserted as two
+ // separate non-empty groups -- rather than one merged list -- so that if
+ // either selector stops matching (e.g. `pvtCornerLabel` is dropped from
+ // the column-attribute name cell), that group's absence fails the test
+ // instead of silently leaving only the other group's cells checked.
+ const columnCornerCells = container.querySelectorAll(
+ 'thead th.pvtCornerLabel',
+ );
+ const rowHeaderCornerCells = container.querySelectorAll(
+ 'thead tr.pvtRowHeaderRow th.pvtAxisLabel',
+ );
+ expect(columnCornerCells.length).toBeGreaterThan(0);
+ expect(rowHeaderCornerCells.length).toBeGreaterThan(0);
+ const cornerCells = [...columnCornerCells, ...rowHeaderCornerCells];
+ // The frozen block in each column-header row has to cover the same
+ // columns as the frozen row label below it (its own column plus the
+ // padding column), otherwise the uncovered strip shows column labels
+ // scrolling through the corner.
+ const rowLabelSpan = (rowLabelCell as HTMLTableCellElement).colSpan;
+ expect(rowLabelSpan).toBe(2);
+ columnCornerCells.forEach(cell => {
+ expect((cell as HTMLTableCellElement).colSpan).toBe(rowLabelSpan);
+ });
+ cornerCells.forEach(cell => {
+ const style = getComputedStyle(cell);
+ expect(style.position).toBe('sticky');
+ expect(style.top).toBe('0px');
+ expect(style.left).toBe('0px');
+ // Assert the declared value, not just a positive number: with no
+ // z-index rule applied, jsdom's getComputedStyle resolves to '' and
+ // Number('') is 0, so a bare `> 0` check would still pass.
+ expect(style.zIndex).toBe('1');
+ });
+
+ // The sticky thead is its own stacking context, so the corner cell's
+ // z-index can't outrank the row labels by itself. The thead as a whole
+ // has to sit above the frozen row-label column.
+ const thead = container.querySelector('thead');
+ expect(thead).toBeInTheDocument();
+ expect(getComputedStyle(thead as Element).zIndex).toBe('2');
+ expect(rowLabelStyle.zIndex).toBe('1');
+});
+
+test('keeps the sticky totals row above the frozen row-label column', () => {
+ const transformedProps = {
+ ...transformProps(testData.withColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const rowLabelCell = container.querySelector('tbody th.pvtRowLabel');
+ const totalsRow = container.querySelector('tbody tr.pvtRowTotals');
+ expect(rowLabelCell).toBeInTheDocument();
+ expect(totalsRow).toBeInTheDocument();
+ expect(getComputedStyle(totalsRow as Element).zIndex).toBe('2');
+ expect(getComputedStyle(rowLabelCell as Element).zIndex).toBe('1');
+
+ // The totals row's leading label freezes at the left edge alongside the
+ // body row labels, and sits above the totals values in its own row.
+ const totalsLabel = totalsRow?.querySelector('th.pvtRowTotalLabel');
+ expect(totalsLabel).toBeInTheDocument();
+ const totalsLabelStyle = getComputedStyle(totalsLabel as Element);
+ expect(totalsLabelStyle.position).toBe('sticky');
+ expect(totalsLabelStyle.left).toBe('0px');
+ // Assert the declared value rather than `> 0`: with no z-index rule
+ // applied, jsdom resolves getComputedStyle(...).zIndex to '', and
+ // Number('') is 0, so a bare positivity check would still pass.
+ expect(totalsLabelStyle.zIndex).toBe('1');
+ expect((totalsLabel as HTMLTableCellElement).colSpan).toBe(
+ (rowLabelCell as HTMLTableCellElement).colSpan,
+ );
+});
+
+test('lets the active (cross-filter) highlight win over the frozen row-label background', () => {
+ const transformedProps = {
+ ...transformProps(testData.withoutColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ // Computed colors come back normalized (rgb()), so run the theme tokens
+ // through the same normalization before comparing.
+ const normalizeColor = (color: string) => {
+ const probe = document.createElement('div');
+ probe.style.backgroundColor = color;
+ return probe.style.backgroundColor;
+ };
+
+ const rowLabelCell = container.querySelector('tbody th.pvtRowLabel');
+ expect(rowLabelCell).toBeInTheDocument();
+ expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe(
+ normalizeColor(supersetTheme.colorBgBase),
+ );
+
+ rowLabelCell?.classList.add('active');
+ expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe(
+ normalizeColor(supersetTheme.colorPrimaryBg),
+ );
+});
+
+test('does not freeze any header cell when the pivot has column dimensions but no row dimensions', () => {
+ const transformedProps = {
+ ...transformProps(testData.columnsOnly),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ // Without row dimensions there is no row-header row, so the leading
+ // cell of the last header row is the column attribute name. It must
+ // scroll with its column rather than being treated as a corner cell.
+ expect(container.querySelector('thead tr.pvtRowHeaderRow')).toBeNull();
+ const headerCells = container.querySelectorAll('thead th');
+ expect(headerCells.length).toBeGreaterThan(0);
+ headerCells.forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+});
+
+test('does not freeze the row-label column or its corner cell(s) with more than one row dimension', () => {
+ // With multiple row attributes, every row-label and corner cell would
+ // freeze at the same left: 0 edge and stack on top of one another, so
+ // freezing is scoped to the single-row-dimension case until a
+ // per-column offset fast-follow lands.
+ const transformedProps = {
+ ...transformProps(testData.groupedRowsWithoutColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const rowLabelCells = container.querySelectorAll('tbody th.pvtRowLabel');
+ expect(rowLabelCells.length).toBeGreaterThan(0);
+ rowLabelCells.forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+
+ const cornerCells = [
Review Comment:
Added a fixture with multiple row dims and a column dim, and split the assertion into two non-empty groups so `pvtCornerLabel` actually gets exercised.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4131754889
##########
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:
Good catch — `left: 0` wasn't gated at all, so multi-level row headers did stack on the same edge. Scoped the sticky positioning (`pvtRowLabel`, the corner cells, `pvtRowTotalLabel`) to the single-row-dimension case; with more than one it falls back to static like before this PR.
##########
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:
Right, that rule can't match anymore after the corner-cell merge — removed it as dead code.
##########
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:
Fixed — the comment now calls out the subtotal-row case separately, where the extra span lands on `attrValuePaddingCell` instead of the last label cell.
##########
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:
Split the corner-cell length check into two separate non-empty assertions (so either selector going dark on its own fails the test), and switched the z-index checks to assert the declared value instead of `> 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: 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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-6056060887
## CodeAnt PR Risk: Low Risk
- The PR appears safe to merge; sticky row labels are limited to a single row dimension, and the changed layout has targeted tests.
- For pivots with column dimensions but no row dimensions, the row-header cell now displays “Total” where it was previously blank; confirm this is intentional.
**Assessed commit:** `8529ec044d1a`
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4153447573
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx:
##########
@@ -0,0 +1,282 @@
+/**
+ * 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. Asserted as two
+ // separate non-empty groups -- rather than one merged list -- so that if
+ // either selector stops matching (e.g. `pvtCornerLabel` is dropped from
+ // the column-attribute name cell), that group's absence fails the test
+ // instead of silently leaving only the other group's cells checked.
+ const columnCornerCells = container.querySelectorAll(
+ 'thead th.pvtCornerLabel',
+ );
+ const rowHeaderCornerCells = container.querySelectorAll(
+ 'thead tr.pvtRowHeaderRow th.pvtAxisLabel',
+ );
+ expect(columnCornerCells.length).toBeGreaterThan(0);
+ expect(rowHeaderCornerCells.length).toBeGreaterThan(0);
+ const cornerCells = [...columnCornerCells, ...rowHeaderCornerCells];
+ // The frozen block in each column-header row has to cover the same
+ // columns as the frozen row label below it (its own column plus the
+ // padding column), otherwise the uncovered strip shows column labels
+ // scrolling through the corner.
+ const rowLabelSpan = (rowLabelCell as HTMLTableCellElement).colSpan;
+ expect(rowLabelSpan).toBe(2);
+ columnCornerCells.forEach(cell => {
+ expect((cell as HTMLTableCellElement).colSpan).toBe(rowLabelSpan);
Review Comment:
With one row and one column dimension, removing the merged row-attribute header’s `colSpan={2}` would leave part of the frozen body-label block uncovered, but these assertions would still pass because only `columnCornerCells` have their span checked. Could this rendered-chart test also assert that the row-header corner cell spans the same two columns as the body label?
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/Styles.test.tsx:
##########
@@ -0,0 +1,282 @@
+/**
+ * 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. Asserted as two
+ // separate non-empty groups -- rather than one merged list -- so that if
+ // either selector stops matching (e.g. `pvtCornerLabel` is dropped from
+ // the column-attribute name cell), that group's absence fails the test
+ // instead of silently leaving only the other group's cells checked.
+ const columnCornerCells = container.querySelectorAll(
+ 'thead th.pvtCornerLabel',
+ );
+ const rowHeaderCornerCells = container.querySelectorAll(
+ 'thead tr.pvtRowHeaderRow th.pvtAxisLabel',
+ );
+ expect(columnCornerCells.length).toBeGreaterThan(0);
+ expect(rowHeaderCornerCells.length).toBeGreaterThan(0);
+ const cornerCells = [...columnCornerCells, ...rowHeaderCornerCells];
+ // The frozen block in each column-header row has to cover the same
+ // columns as the frozen row label below it (its own column plus the
+ // padding column), otherwise the uncovered strip shows column labels
+ // scrolling through the corner.
+ const rowLabelSpan = (rowLabelCell as HTMLTableCellElement).colSpan;
+ expect(rowLabelSpan).toBe(2);
+ columnCornerCells.forEach(cell => {
+ expect((cell as HTMLTableCellElement).colSpan).toBe(rowLabelSpan);
+ });
+ cornerCells.forEach(cell => {
+ const style = getComputedStyle(cell);
+ expect(style.position).toBe('sticky');
+ expect(style.top).toBe('0px');
+ expect(style.left).toBe('0px');
+ // Assert the declared value, not just a positive number: with no
+ // z-index rule applied, jsdom's getComputedStyle resolves to '' and
+ // Number('') is 0, so a bare `> 0` check would still pass.
+ expect(style.zIndex).toBe('1');
+ });
+
+ // The sticky thead is its own stacking context, so the corner cell's
+ // z-index can't outrank the row labels by itself. The thead as a whole
+ // has to sit above the frozen row-label column.
+ const thead = container.querySelector('thead');
+ expect(thead).toBeInTheDocument();
+ expect(getComputedStyle(thead as Element).zIndex).toBe('2');
+ expect(rowLabelStyle.zIndex).toBe('1');
+});
+
+test('keeps the sticky totals row above the frozen row-label column', () => {
+ const transformedProps = {
+ ...transformProps(testData.withColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const rowLabelCell = container.querySelector('tbody th.pvtRowLabel');
+ const totalsRow = container.querySelector('tbody tr.pvtRowTotals');
+ expect(rowLabelCell).toBeInTheDocument();
+ expect(totalsRow).toBeInTheDocument();
+ expect(getComputedStyle(totalsRow as Element).zIndex).toBe('2');
+ expect(getComputedStyle(rowLabelCell as Element).zIndex).toBe('1');
+
+ // The totals row's leading label freezes at the left edge alongside the
+ // body row labels, and sits above the totals values in its own row.
+ const totalsLabel = totalsRow?.querySelector('th.pvtRowTotalLabel');
+ expect(totalsLabel).toBeInTheDocument();
+ const totalsLabelStyle = getComputedStyle(totalsLabel as Element);
+ expect(totalsLabelStyle.position).toBe('sticky');
+ expect(totalsLabelStyle.left).toBe('0px');
+ // Assert the declared value rather than `> 0`: with no z-index rule
+ // applied, jsdom resolves getComputedStyle(...).zIndex to '', and
+ // Number('') is 0, so a bare positivity check would still pass.
+ expect(totalsLabelStyle.zIndex).toBe('1');
+ expect((totalsLabel as HTMLTableCellElement).colSpan).toBe(
+ (rowLabelCell as HTMLTableCellElement).colSpan,
+ );
+});
+
+test('lets the active (cross-filter) highlight win over the frozen row-label background', () => {
+ const transformedProps = {
+ ...transformProps(testData.withoutColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ // Computed colors come back normalized (rgb()), so run the theme tokens
+ // through the same normalization before comparing.
+ const normalizeColor = (color: string) => {
+ const probe = document.createElement('div');
+ probe.style.backgroundColor = color;
+ return probe.style.backgroundColor;
+ };
+
+ const rowLabelCell = container.querySelector('tbody th.pvtRowLabel');
+ expect(rowLabelCell).toBeInTheDocument();
+ expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe(
+ normalizeColor(supersetTheme.colorBgBase),
+ );
+
+ rowLabelCell?.classList.add('active');
+ expect(getComputedStyle(rowLabelCell as Element).backgroundColor).toBe(
+ normalizeColor(supersetTheme.colorPrimaryBg),
+ );
+});
+
+test('does not freeze any header cell when the pivot has column dimensions but no row dimensions', () => {
+ const transformedProps = {
+ ...transformProps(testData.columnsOnly),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ // Without row dimensions there is no row-header row, so the leading
+ // cell of the last header row is the column attribute name. It must
+ // scroll with its column rather than being treated as a corner cell.
+ expect(container.querySelector('thead tr.pvtRowHeaderRow')).toBeNull();
+ const headerCells = container.querySelectorAll('thead th');
+ expect(headerCells.length).toBeGreaterThan(0);
+ headerCells.forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+});
+
+test('does not freeze the row-label column or its corner cell(s) with more than one row dimension', () => {
+ // With multiple row attributes, every row-label and corner cell would
+ // freeze at the same left: 0 edge and stack on top of one another, so
+ // freezing is scoped to the single-row-dimension case until a
+ // per-column offset fast-follow lands. Uses a fixture with both multiple
+ // row dimensions and a column dimension (unlike groupedRowsWithoutColTotals,
+ // which has no column dimension) so the pvtCornerLabel group below is
+ // actually non-empty and its non-sticky assertion is exercised, rather
+ // than passing vacuously.
+ const transformedProps = {
+ ...transformProps(testData.groupedRowsWithColumnDim),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const rowLabelCells = container.querySelectorAll('tbody th.pvtRowLabel');
+ expect(rowLabelCells.length).toBeGreaterThan(0);
+ rowLabelCells.forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+
+ // Asserted as two separate non-empty groups, mirroring the
+ // sticky-positioning test above, so that if either selector stops
+ // matching, that group's absence fails the test instead of silently
+ // leaving only the other group's cells checked.
+ const columnCornerCells = container.querySelectorAll(
+ 'thead th.pvtCornerLabel',
+ );
+ const rowHeaderCornerCells = container.querySelectorAll(
+ 'thead tr.pvtRowHeaderRow th.pvtAxisLabel',
+ );
+ expect(columnCornerCells.length).toBeGreaterThan(0);
+ expect(rowHeaderCornerCells.length).toBeGreaterThan(0);
+ [...columnCornerCells, ...rowHeaderCornerCells].forEach(cell => {
+ expect(getComputedStyle(cell).position).not.toBe('sticky');
+ });
+});
+
+test('does not leak sticky positioning onto the totals label from its row with more than one row dimension', () => {
+ // th.pvtRowTotalLabel's direct parent is tr.pvtRowTotals, which is
+ // unconditionally sticky (it freezes to the bottom edge regardless of
+ // row-dimension count). The non-sticky branch here must resolve to an
+ // explicit 'static' rather than 'inherit', or the label would pick up
+ // the row's own sticky position and stay pinned at left: 0 anyway.
+ const transformedProps = {
+ ...transformProps(testData.groupedRowsWithColTotals),
+ margin: 32,
+ legacy_order_by: null,
+ order_desc: false,
+ };
+ const { container } = render(
+ ProviderWrapper({
+ children: <PivotTableChart {...transformedProps} />,
+ }),
+ );
+
+ const totalsLabel = container.querySelector(
+ 'tbody tr.pvtRowTotals th.pvtRowTotalLabel',
+ );
+ expect(totalsLabel).toBeInTheDocument();
+ expect(getComputedStyle(totalsLabel as Element).position).toBe('static');
+});
+
+test('does not sticky-position the row-label column or corner cell(s) in dashboard edit mode', () => {
+ // TableRenderers detects dashboard edit mode by looking for this class
+ // on the document, rather than via a prop.
+ const editingMarker = document.createElement('div');
+ editingMarker.className = 'dashboard--editing';
+ document.body.appendChild(editingMarker);
+
+ try {
+ 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();
+ expect(getComputedStyle(rowLabelCell as Element).position).not.toBe(
Review Comment:
This edit-mode test only checks the body-label guard, so a regression in either independent corner-cell or totals-label guard would leave an orphan frozen cell during editing without failing the test. Could it render the totals-enabled fixture and assert both non-empty corner groups are non-sticky and the totals label is `static`?
##########
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 body-cell example was already possible before this change, but the merge newly places the “Country” axis-name header under the spanning “Year” header; `role="grid"` still preserves native header roles and spans ([ARIA in HTML](https://www.w3.org/TR/html-aria/#el-th)). Could this merge avoid that unrelated header association without requiring a broader grid accessibility rewrite?
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3963895146
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -33,10 +33,30 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
line-height: 1.4;
}
+ /* The sticky thead and totals row each form their own stacking
+ * context, so a z-index on a cell inside them can't outrank the
+ * frozen row labels (z-index 1) in the body. The bands themselves
+ * carry the z-index that keeps them painting over row labels
+ * scrolling underneath. */
table thead {
background-color: ${theme.colorBgBase};
position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
top: 0;
+ z-index: 2;
+ }
+
+ /* Corner cell(s) sitting above the frozen row-label column: the
+ * placeholder cell spanning the column-attribute rows, and the
+ * row-attribute name cell(s) in the row-header row (which only
+ * renders when there are row dimensions). The z-index keeps them
+ * over the column labels scrolling underneath within the thead. */
+ table.pvtTable thead tr:first-of-type th[aria-hidden='true'],
Review Comment:
With both row and column dimensions, `spaceCell` is only `rowAttrs.length` columns wide while the merged row-header/body label spans one more. This selector freezes only that narrower spacer, so the column-attribute header still scrolls through the remaining corner strip. Should the full leading block in the column-header rows match the merged span?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -55,6 +75,7 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr.pvtRowTotals {
Review Comment:
With column totals enabled, the totals row's leading `pvtRowTotalLabel` still scrolls horizontally while body row labels are frozen. At a horizontal offset it leaves the left edge and the totals values, now above labels at z-index 2, paint over that column. Should the leading totals cell also stick at `left: 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: 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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4141820492
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -63,6 +100,19 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
background-color: ${theme.colorBgBase};
}
+ /* The totals row's leading label freezes at the left edge like the
+ * body row labels above it. The z-index keeps it over the totals
+ * values scrolling underneath within the row's own stacking context.
+ * Scoped to the single-row-dimension case for the same reason as the
+ * corner cells above. */
+ table.pvtTable tbody tr.pvtRowTotals th.pvtRowTotalLabel {
+ position: ${
+ isDashboardEditMode || !canFreezeRowLabels ? 'inherit' : 'sticky'
Review Comment:
Good catch. Fixed by having that branch use `static` instead of `inherit`: `th.pvtRowTotalLabel`'s direct parent is `tr.pvtRowTotals`, which is always sticky, so `inherit` was picking that up regardless of row-dimension count.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5933689079
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3939109011
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +134,10 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
+ position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
+ left: 0;
+ z-index: 1;
+ background-color: ${theme.colorBgBase};
}
Review Comment:
**Suggestion:** For pivots with multiple row attributes, every frozen label cell uses `left: 0`, so the row-label columns overlap instead of retaining their separate horizontal positions. [css layout issue]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4e4f09980d5c4e1eb8aadb1bade6324b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4e4f09980d5c4e1eb8aadb1bade6324b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts
**Line:** 135:141
**Comment:**
*Css Layout Issue: For pivots with multiple row attributes, every frozen label cell uses `left: 0`, so the row-label columns overlap instead of retaining their separate horizontal positions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43917&comment_hash=4a330a065584ea491c6a23a2bbbf950f31c6b043a34b8648b685ed2f2bd8fa65&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43917&comment_hash=4a330a065584ea491c6a23a2bbbf950f31c6b043a34b8648b685ed2f2bd8fa65&reaction=dislike'>👎</a>
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5548696739
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43917?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 79.59%. Comparing base ([`a3aba2a`](https://app.codecov.io/gh/apache/superset/commit/a3aba2a16a307da270b503693559dc1ec638516d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`12f7e68`](https://app.codecov.io/gh/apache/superset/commit/12f7e6859c6c31cc5a20837594ab7f44e1bfaa52?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43917 +/- ##
==========================================
- Coverage 79.59% 79.59% -0.01%
==========================================
Files 2900 2900
Lines 168843 168845 +2
Branches 39173 39175 +2
==========================================
- Hits 134398 134397 -1
- Misses 31945 31948 +3
Partials 2500 2500
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43917/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/43917/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.15% <100.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43917?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3939707854
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +134,10 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
+ position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
+ left: 0;
+ z-index: 1;
+ background-color: ${theme.colorBgBase};
}
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag the fixed `left: 0` positioning for multi-level row headers in this styles file; per-column offsets based on rendered widths are intentionally deferred to a follow-up.
**Applied to:**
- `superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43917:
URL: https://github.com/apache/superset/pull/43917#issuecomment-5550883175
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3943091282
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -39,6 +39,20 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
top: 0;
}
+ /* Corner cell(s) sitting above the frozen row-label column: the
+ * placeholder cell spanning the column-attribute rows, and the
+ * row-attribute name cell(s) in the final header row. Needs a
+ * higher z-index than .pvtRowLabel so it stays on top when both
+ * stick at the same scroll position. */
+ table.pvtTable thead tr:first-of-type th[aria-hidden='true'],
+ table.pvtTable thead tr:last-of-type th.pvtAxisLabel {
+ position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
+ top: 0;
+ left: 0;
+ z-index: 2;
Review Comment:
Good catch, thanks @sadpandajoe. The corner cell's `z-index` was trapped inside the thead's own stacking context. Moved the `z-index` onto `thead` (and `tr.pvtRowTotals`, same problem on the bottom edge) so the bands themselves outrank the row labels.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +134,10 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
+ position: ${isDashboardEditMode ? 'inherit' : 'sticky'};
+ left: 0;
+ z-index: 1;
+ background-color: ${theme.colorBgBase};
}
Review Comment:
Yep, they should. Restated `.active` and `.hoverable:hover` for the frozen cells so they win again. The hover tint goes on as a `background-image` over the opaque base, otherwise the translucent fill token lets scrolled-under cells show through.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -39,6 +39,20 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
top: 0;
}
+ /* Corner cell(s) sitting above the frozen row-label column: the
+ * placeholder cell spanning the column-attribute rows, and the
+ * row-attribute name cell(s) in the final header row. Needs a
+ * higher z-index than .pvtRowLabel so it stays on top when both
+ * stick at the same scroll position. */
+ table.pvtTable thead tr:first-of-type th[aria-hidden='true'],
+ table.pvtTable thead tr:last-of-type th.pvtAxisLabel {
Review Comment:
Right, with no row dimensions that selector was grabbing the column attribute name. The renderer tags the row-header `<tr>` as `pvtRowHeaderRow` (it only renders when there are row dims) and the corner rule keys off that. Added a columns-only fixture that checks nothing in the thead freezes.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3943131664
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -39,6 +39,20 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
top: 0;
}
+ /* Corner cell(s) sitting above the frozen row-label column: the
+ * placeholder cell spanning the column-attribute rows, and the
+ * row-attribute name cell(s) in the final header row. Needs a
+ * higher z-index than .pvtRowLabel so it stays on top when both
+ * stick at the same scroll position. */
+ table.pvtTable thead tr:first-of-type th[aria-hidden='true'],
+ table.pvtTable thead tr:last-of-type th.pvtAxisLabel {
Review Comment:
Good catch, @sadpandajoe. Folded the placeholder cell into the row-attribute-name cell with a matching colSpan, so the frozen header block spans the same two columns as the row-label cell below it instead of freezing them separately.
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r3941616595
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -39,6 +39,20 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
top: 0;
}
+ /* Corner cell(s) sitting above the frozen row-label column: the
+ * placeholder cell spanning the column-attribute rows, and the
+ * row-attribute name cell(s) in the final header row. Needs a
+ * higher z-index than .pvtRowLabel so it stays on top when both
+ * stick at the same scroll position. */
+ table.pvtTable thead tr:first-of-type th[aria-hidden='true'],
+ table.pvtTable thead tr:last-of-type th.pvtAxisLabel {
Review Comment:
With one row dimension and any column dimension, the final row label spans both leading grid columns, but this only freezes the first matching header cell. The adjacent header cell scrolls away while the body cell remains frozen, so the header no longer lines up with its column. Should the renderer mark the complete corner block and freeze that block rather than infer individual cells from row position?
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #43917:
URL: https://github.com/apache/superset/pull/43917
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4171138712
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/Styles.ts:
##########
@@ -120,6 +165,29 @@ export const Styles = styled.div<{ isDashboardEditMode: boolean }>`
table.pvtTable tbody tr th.pvtRowLabel {
vertical-align: baseline;
+ position: ${
Review Comment:
With one row dimension containing a long unbroken value (for example, a 64-character request ID) in a narrow chart, the frozen label can be wider than the scroll area and cover every metric cell even at maximum horizontal scroll, making the values unreadable and unclickable. Could the frozen block be width-constrained or freezing disabled when it consumes the scrollport?
--
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
Re: [PR] fix(plugin-chart-pivot-table): sticky-position row labels and corner header cells [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43917:
URL: https://github.com/apache/superset/pull/43917#discussion_r4139052121
##########
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:
This whole table uses `role="grid"`, not native table semantics, and nothing in it sets `scope`/`headers` anywhere else either, so bolting it onto just this one cell wouldn't really fix the association model. A real a11y pass on the grid's header relationships feels like separate work from a sticky-positioning fix.
##########
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
Review Comment:
Fixed, both comments now call out that this only holds for a single row dimension since `canFreezeRowLabels` falls back to non-sticky otherwise.
--
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