Copilot commented on code in PR #37211:
URL: https://github.com/apache/superset/pull/37211#discussion_r2713899153
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -47,39 +46,44 @@ const containerStyle = (theme: SupersetTheme) => css`
display: flex;
&& > .filter-clear-all-button {
- color: ${theme.colorTextSecondary};
+ color: ${theme.colors.grayscale.base};
margin-left: 0;
&:hover {
- color: ${theme.colorPrimaryText};
+ color: ${theme.colors.primary.dark1};
}
&[disabled],
&[disabled]:hover {
- color: ${theme.colorTextDisabled};
+ color: ${theme.colors.grayscale.light1};
}
}
`;
const verticalStyle = (theme: SupersetTheme, width: number) => css`
flex-direction: column;
align-items: center;
+ pointer-events: none;
position: fixed;
z-index: 100;
// filter bar width minus 1px for border
width: ${width - 1}px;
bottom: 0;
- padding: ${theme.sizeUnit * 4}px;
- padding-top: ${theme.sizeUnit * 6}px;
+ padding: ${theme.gridUnit * 4}px;
+ padding-top: ${theme.gridUnit * 6}px;
background: linear-gradient(
- ${tinycolor(theme.colorBgLayout).setAlpha(0).toRgbString()},
- ${theme.colorBgContainer} 20%
+ ${rgba(theme.colors.grayscale.light5, 0)},
+ ${theme.colors.grayscale.light5} ${theme.opacity.mediumLight}
Review Comment:
The gradient background in the vertical filter bar uses rgba with
theme.opacity.mediumLight, but the original used a percentage value (20%). The
property `theme.opacity.mediumLight` may not exist in the theme object, which
would cause a runtime error. Verify that this theme property exists, or use a
literal opacity value.
```suggestion
${theme.colors.grayscale.light5} 20%
```
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -16,30 +16,27 @@
* specific language governing permissions and limitations
* under the License.
*/
-import {
+import React, {
forwardRef,
ReactNode,
useContext,
useEffect,
useRef,
useState,
} from 'react';
-import { t } from '@apache-superset/core';
-import { getExtensionsRegistry, QueryData } from '@superset-ui/core';
-import { css, styled, SupersetTheme, useTheme } from
'@apache-superset/core/ui';
+import { css, getExtensionsRegistry, styled, t } from '@superset-ui/core';
import { useUiConfig } from 'src/components/UiConfigContext';
-import { isEmbedded } from 'src/dashboard/util/isEmbedded';
-import { Tooltip, EditableTitle, Icons } from '@superset-ui/core/components';
+import { Tooltip } from 'src/components/Tooltip';
import { useSelector } from 'react-redux';
-import SliceHeaderControls from 'src/dashboard/components/SliceHeaderControls';
-import { SliceHeaderControlsProps } from
'src/dashboard/components/SliceHeaderControls/types';
+import EditableTitle from 'src/components/EditableTitle';
Review Comment:
The import path for EditableTitle is incorrect. The component should be
imported from '@superset-ui/core' or '@superset-ui/core/components' where it is
exported (as shown in line 104 of
packages/superset-ui-core/src/components/index.ts), not from
'src/components/EditableTitle' which doesn't exist. This will cause a module
resolution error.
```suggestion
import EditableTitle from '@superset-ui/core';
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -16,19 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { useMemo } from 'react';
-import { t } from '@apache-superset/core';
+import React, { useMemo } from 'react';
import {
+ css,
DataMaskState,
DataMaskStateWithId,
+ t,
isDefined,
- ChartCustomization,
- ChartCustomizationDivider,
+ SupersetTheme,
} from '@superset-ui/core';
-import { css, SupersetTheme, styled } from '@apache-superset/core/ui';
-import { Button } from '@superset-ui/core/components';
+import Button from 'src/components/Button';
Review Comment:
The import path for Button is incorrect. The component should be imported
from '@superset-ui/core' or '@superset-ui/core/components' where it is actually
exported, not from 'src/components/Button' which doesn't exist in the codebase.
This will cause a module resolution error.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -107,63 +111,55 @@ const ActionButtons = ({
dataMaskSelected,
isApplyDisabled,
filterBarOrientation = FilterBarOrientation.Vertical,
- chartCustomizationItems,
}: ActionButtonsProps) => {
const isClearAllEnabled = useMemo(() => {
- const hasSelectedChanges = Object.entries(dataMaskSelected).some(
- ([, mask]) => {
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- },
+ // Check both selected (pending) and applied filters to determine if clear
is available
+ const hasSelectedValues = Object.values(dataMaskSelected).some(
+ mask => isDefined(mask.filterState?.value),
);
-
- const hasAppliedChanges = Object.entries(dataMaskApplied).some(
- ([, mask]) => {
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- },
+ const hasAppliedValues = Object.values(dataMaskApplied).some(
+ mask => isDefined(mask.filterState?.value),
);
-
- const hasChartCustomizations = chartCustomizationItems?.some(item => {
- if (item.removed) return false;
- const mask = dataMaskApplied[item.id] || dataMaskSelected[item.id];
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- });
-
- return hasSelectedChanges || hasAppliedChanges || hasChartCustomizations;
- }, [dataMaskSelected, dataMaskApplied, chartCustomizationItems]);
+ return hasSelectedValues || hasAppliedValues;
+ }, [dataMaskSelected, dataMaskApplied]);
const isVertical = filterBarOrientation === FilterBarOrientation.Vertical;
return (
- <ButtonsContainer
- isVertical={isVertical}
- width={width}
+ <div
+ css={(theme: SupersetTheme) => [
+ containerStyle(theme),
+ isVertical ? verticalStyle(theme, width) : horizontalStyle(theme),
+ ]}
Review Comment:
The styled component `ButtonsContainer` has been removed and replaced with
inline `css` prop usage. However, this change introduces potential
maintainability issues. The css function is being passed an array of style
functions, which is less type-safe and harder to debug than the previous
styled-component approach. Consider keeping the styled component for better
separation of concerns and type safety.
##########
superset-frontend/src/components/Accessibility/StatusAnnouncer.test.tsx:
##########
@@ -0,0 +1,657 @@
+/**
+ * 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 { act, render, screen, waitFor } from 'spec/helpers/testing-library';
Review Comment:
Unused import waitFor.
```suggestion
import { act, render, screen } from 'spec/helpers/testing-library';
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -107,63 +111,55 @@ const ActionButtons = ({
dataMaskSelected,
isApplyDisabled,
filterBarOrientation = FilterBarOrientation.Vertical,
- chartCustomizationItems,
}: ActionButtonsProps) => {
const isClearAllEnabled = useMemo(() => {
- const hasSelectedChanges = Object.entries(dataMaskSelected).some(
- ([, mask]) => {
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- },
+ // Check both selected (pending) and applied filters to determine if clear
is available
+ const hasSelectedValues = Object.values(dataMaskSelected).some(
+ mask => isDefined(mask.filterState?.value),
);
-
- const hasAppliedChanges = Object.entries(dataMaskApplied).some(
- ([, mask]) => {
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- },
+ const hasAppliedValues = Object.values(dataMaskApplied).some(
+ mask => isDefined(mask.filterState?.value),
);
-
- const hasChartCustomizations = chartCustomizationItems?.some(item => {
- if (item.removed) return false;
- const mask = dataMaskApplied[item.id] || dataMaskSelected[item.id];
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- });
-
- return hasSelectedChanges || hasAppliedChanges || hasChartCustomizations;
- }, [dataMaskSelected, dataMaskApplied, chartCustomizationItems]);
+ return hasSelectedValues || hasAppliedValues;
+ }, [dataMaskSelected, dataMaskApplied]);
Review Comment:
The `chartCustomizationItems` prop and all related logic have been removed
from the ActionButtons component. This includes the logic for checking chart
customizations when determining if the "Clear all" button should be enabled.
This is a breaking change that affects the component's API and functionality,
contradicting the PR description's claim of "No breaking changes to existing
functionality".
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -16,19 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { useMemo } from 'react';
-import { t } from '@apache-superset/core';
+import React, { useMemo } from 'react';
import {
+ css,
DataMaskState,
DataMaskStateWithId,
+ t,
isDefined,
- ChartCustomization,
- ChartCustomizationDivider,
+ SupersetTheme,
} from '@superset-ui/core';
-import { css, SupersetTheme, styled } from '@apache-superset/core/ui';
-import { Button } from '@superset-ui/core/components';
+import Button from 'src/components/Button';
+import { Tooltip } from 'src/components/Tooltip';
Review Comment:
The import path for Tooltip is incorrect. The component should be imported
from '@superset-ui/core' or '@superset-ui/core/components' where it is actually
exported, not from 'src/components/Tooltip' which doesn't exist in the
codebase. This will cause a module resolution error.
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -165,200 +171,147 @@ const SliceHeader = forwardRef<HTMLDivElement,
SliceHeaderProps>(
formData,
width,
height,
- exportPivotExcel = () => ({}),
},
ref,
) => {
- const SliceHeaderExtension = extensionsRegistry.get(
- 'dashboard.slice.header',
- );
- const uiConfig = useUiConfig();
- const shouldShowRowLimitWarning =
- !isEmbedded() || uiConfig.showRowLimitWarning;
- const dashboardPageId = useContext(DashboardPageIdContext);
- const [headerTooltip, setHeaderTooltip] = useState<ReactNode | null>(null);
- const headerRef = useRef<HTMLDivElement>(null);
- // TODO: change to indicator field after it will be implemented
- const crossFilterValue = useSelector<RootState, any>(
- state => state.dataMask[slice?.slice_id]?.filterState?.value,
- );
- const isCrossFiltersEnabled = useSelector<RootState, boolean>(
- ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
- );
-
- const firstQueryResponse = useSelector<RootState, QueryData | undefined>(
- state => state.charts[slice.slice_id].queriesResponse?.[0],
- );
-
- const theme = useTheme();
-
- const rowLimit = Number(formData.row_limit || -1);
- const sqlRowCount = Number(firstQueryResponse?.sql_rowcount || 0);
+ const SliceHeaderExtension =
extensionsRegistry.get('dashboard.slice.header');
+ const uiConfig = useUiConfig();
+ const dashboardPageId = useContext(DashboardPageIdContext);
+ const [headerTooltip, setHeaderTooltip] = useState<ReactNode | null>(null);
+ const headerRef = useRef<HTMLDivElement>(null);
+ // TODO: change to indicator field after it will be implemented
+ const crossFilterValue = useSelector<RootState, any>(
+ state => state.dataMask[slice?.slice_id]?.filterState?.value,
+ );
+ const isCrossFiltersEnabled = useSelector<RootState, boolean>(
+ ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
+ );
- const canExplore = !editMode && supersetCanExplore;
+ const canExplore = !editMode && supersetCanExplore;
- useEffect(() => {
- const headerElement = headerRef.current;
- if (canExplore) {
- setHeaderTooltip(getSliceHeaderTooltip(sliceName));
- } else if (
- headerElement &&
- (headerElement.scrollWidth > headerElement.offsetWidth ||
- headerElement.scrollHeight > headerElement.offsetHeight)
- ) {
- setHeaderTooltip(sliceName ?? null);
- } else {
- setHeaderTooltip(null);
- }
- }, [sliceName, width, height, canExplore]);
-
- const exploreUrl =
`/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${slice.slice_id}`;
+ useEffect(() => {
+ const headerElement = headerRef.current;
+ if (canExplore) {
+ setHeaderTooltip(getSliceHeaderTooltip(sliceName));
+ } else if (
+ headerElement &&
+ (headerElement.scrollWidth > headerElement.offsetWidth ||
+ headerElement.scrollHeight > headerElement.offsetHeight)
+ ) {
+ setHeaderTooltip(sliceName ?? null);
+ } else {
+ setHeaderTooltip(null);
+ }
+ }, [sliceName, width, height, canExplore]);
- const renderExploreLink = (title: string) => (
- <Link
- to={exploreUrl}
- css={(theme: SupersetTheme) => css`
- color: ${theme.colorText};
- text-decoration: none;
- :hover {
- text-decoration: underline;
- }
- display: inline-block;
- `}
- >
- {title}
- </Link>
- );
+ const exploreUrl =
`/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${slice.slice_id}`;
- return (
- <ChartHeaderStyles data-test="slice-header" ref={ref}>
- <div className="header-title" ref={headerRef}>
+ return (
+ <ChartHeaderStyles data-test="slice-header" ref={ref}>
+ <div className="header-title" ref={headerRef}>
+ {/* WCAG 1.3.1: Chart title as semantic heading for screen readers */}
+ <h2>
<Tooltip title={headerTooltip}>
- {/* this div ensures the hover event triggers correctly and
prevents flickering */}
- <div>
- <EditableTitle
- title={
- sliceName ||
- (editMode
- ? '---' // this makes an empty title clickable
- : '')
- }
- canEdit={editMode}
- onSaveTitle={updateSliceName}
- showTooltip={false}
- renderLink={
- canExplore && exploreUrl ? renderExploreLink : undefined
- }
- />
- </div>
+ <EditableTitle
+ title={
+ sliceName ||
+ (editMode
+ ? '---' // this makes an empty title clickable
+ : '')
+ }
+ canEdit={editMode}
+ onSaveTitle={updateSliceName}
+ showTooltip={false}
+ url={canExplore ? exploreUrl : undefined}
+ />
</Tooltip>
- {!!Object.values(annotationQuery).length && (
- <Tooltip
- id="annotations-loading-tooltip"
- placement="top"
- title={annotationsLoading}
- >
- <Icons.ReloadOutlined
- className="warning"
- aria-label={annotationsLoading}
+ </h2>
Review Comment:
The semantic `<h2>` element wrapping the chart title is a good accessibility
improvement (WCAG 1.3.1). However, the nested structure
`<h2><Tooltip><EditableTitle></EditableTitle></Tooltip></h2>` may cause issues.
EditableTitle likely renders its own elements (divs, spans, or inputs), and
nesting these inside an h2 could create invalid HTML if EditableTitle renders
block-level elements. Consider reviewing the EditableTitle component's output
to ensure it only renders inline or phrasing content compatible with heading
elements.
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -16,30 +16,27 @@
* specific language governing permissions and limitations
* under the License.
*/
-import {
+import React, {
forwardRef,
ReactNode,
useContext,
useEffect,
useRef,
useState,
} from 'react';
-import { t } from '@apache-superset/core';
-import { getExtensionsRegistry, QueryData } from '@superset-ui/core';
-import { css, styled, SupersetTheme, useTheme } from
'@apache-superset/core/ui';
+import { css, getExtensionsRegistry, styled, t } from '@superset-ui/core';
import { useUiConfig } from 'src/components/UiConfigContext';
-import { isEmbedded } from 'src/dashboard/util/isEmbedded';
-import { Tooltip, EditableTitle, Icons } from '@superset-ui/core/components';
+import { Tooltip } from 'src/components/Tooltip';
Review Comment:
The import path for Tooltip is incorrect. The component should be imported
from '@superset-ui/core' or '@superset-ui/core/components' where it is
exported, not from 'src/components/Tooltip' which doesn't exist. This will
cause a module resolution error.
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -16,30 +16,27 @@
* specific language governing permissions and limitations
* under the License.
*/
-import {
+import React, {
forwardRef,
ReactNode,
useContext,
useEffect,
useRef,
useState,
} from 'react';
-import { t } from '@apache-superset/core';
-import { getExtensionsRegistry, QueryData } from '@superset-ui/core';
-import { css, styled, SupersetTheme, useTheme } from
'@apache-superset/core/ui';
+import { css, getExtensionsRegistry, styled, t } from '@superset-ui/core';
import { useUiConfig } from 'src/components/UiConfigContext';
-import { isEmbedded } from 'src/dashboard/util/isEmbedded';
-import { Tooltip, EditableTitle, Icons } from '@superset-ui/core/components';
+import { Tooltip } from 'src/components/Tooltip';
import { useSelector } from 'react-redux';
-import SliceHeaderControls from 'src/dashboard/components/SliceHeaderControls';
-import { SliceHeaderControlsProps } from
'src/dashboard/components/SliceHeaderControls/types';
+import EditableTitle from 'src/components/EditableTitle';
+import SliceHeaderControls, {
+ SliceHeaderControlsProps,
+} from 'src/dashboard/components/SliceHeaderControls';
import FiltersBadge from 'src/dashboard/components/FiltersBadge';
-import CustomizationsBadge from 'src/dashboard/components/CustomizationsBadge';
+import Icons from 'src/components/Icons';
Review Comment:
The import path for Icons is incorrect. Icons should be imported from
'@superset-ui/core' or '@superset-ui/core/components' where it is exported, not
from 'src/components/Icons' which doesn't exist. This will cause a module
resolution error. Also note that Icons is a named export, not a default export,
so the import statement should use named import syntax: `import { Icons } from
'@superset-ui/core'`.
##########
superset-frontend/src/components/Accessibility/SkipLink.tsx:
##########
@@ -0,0 +1,91 @@
+/**
+ * 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 React from 'react';
+import { styled } from '@superset-ui/core';
+
+/**
+ * SkipLink - WCAG 2.4.1 Bypass Blocks
+ * Allows keyboard users to skip navigation and jump directly to main content.
+ * The link is visually hidden but becomes visible when focused.
+ */
+
+const StyledSkipLink = styled.a`
+ position: absolute;
+ top: -100px;
+ left: 0;
+ background: ${({ theme }) => theme.colors.primary.dark1};
+ color: ${({ theme }) => theme.colors.grayscale.light5};
+ padding: ${({ theme }) => theme.gridUnit * 3}px ${({ theme }) =>
theme.gridUnit * 4}px;
+ z-index: 10000;
+ text-decoration: none;
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+ font-size: ${({ theme }) => theme.typography.sizes.m}px;
+ border-radius: 0 0 ${({ theme }) => theme.borderRadius}px ${({ theme }) =>
theme.borderRadius}px;
+ transition: top 0.2s ease-in-out;
+
+ &:focus,
+ &:focus-visible {
+ top: 0 !important;
+ outline: 3px solid ${({ theme }) => theme.colors.primary.light1};
+ outline-offset: 2px;
+ }
+
+ &:hover {
+ background: ${({ theme }) => theme.colors.primary.base};
+ }
+`;
+
+interface SkipLinkProps {
+ targetId?: string;
+ children?: React.ReactNode;
+}
+
+const SkipLink: React.FC<SkipLinkProps> = ({
+ targetId = 'main-content',
+ children = 'Skip to main content',
+}) => {
+ const handleClick = (e: React.MouseEvent<HTMLAnchorElement>) => {
+ e.preventDefault();
+ const el = document.getElementById(targetId);
+ if (el) {
+ // Make sure the element is focusable and focus it
+ // Note: We intentionally keep the tabindex to ensure the element
remains focusable
+ // for subsequent keyboard navigation (fixes skip link accessibility)
Review Comment:
The comment states that the tabindex attribute is intentionally kept to
ensure the element remains focusable for subsequent keyboard navigation.
However, setting tabindex="-1" makes the element only programmatically
focusable (not reachable via keyboard Tab navigation). This conflicts with the
stated goal of "subsequent keyboard navigation."
If the intent is to allow keyboard navigation to the target after focus, the
tabindex should remain at "-1" (which is correct for skip link targets), but
the comment is misleading about what this achieves.
```suggestion
// Ensure the target can be programmatically focused for the skip link
// Note: tabindex="-1" makes the element focusable via script without
adding it
// to the Tab order, so users can continue normal keyboard navigation
afterward.
```
##########
superset-frontend/src/components/Accessibility/SkipLink.test.tsx:
##########
@@ -0,0 +1,345 @@
+/**
+ * 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 userEvent from '@testing-library/user-event';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import SkipLink from './SkipLink';
+
+describe('SkipLink', () => {
+ beforeEach(() => {
+ // Clean up any target elements from previous tests
+ const existingTarget = document.getElementById('main-content');
+ if (existingTarget) {
+ existingTarget.remove();
+ }
+ });
+
+ describe('Rendering', () => {
+ test('renders with default "Skip to main content" text', () => {
+ render(<SkipLink />);
+ expect(screen.getByText('Skip to main content')).toBeInTheDocument();
+ });
+
+ test('renders with custom children text', () => {
+ render(<SkipLink>Skip to navigation</SkipLink>);
+ expect(screen.getByText('Skip to navigation')).toBeInTheDocument();
+ });
+
+ test('renders as anchor element with correct href', () => {
+ render(<SkipLink targetId="custom-target" />);
+ const link = screen.getByRole('link');
+ expect(link).toHaveAttribute('href', '#custom-target');
+ });
+
+ test('applies styled-component class', () => {
+ render(<SkipLink />);
+ const link = screen.getByRole('link');
+ expect(link).toHaveClass('a11y-skip-link');
+ });
+ });
+
+ describe('Visual States', () => {
+ test('is positioned off-screen by default (top: -100px)', () => {
+ render(<SkipLink />);
+ const link = screen.getByRole('link');
+ expect(link).toHaveStyleRule('top', '-100px');
+ });
+
+ test('has correct z-index for overlay (10000)', () => {
+ render(<SkipLink />);
+ const link = screen.getByRole('link');
+ expect(link).toHaveStyleRule('z-index', '10000');
+ });
+
+ test('has absolute positioning', () => {
+ render(<SkipLink />);
+ const link = screen.getByRole('link');
+ expect(link).toHaveStyleRule('position', 'absolute');
+ });
+ });
+
+ describe('Focus Behavior', () => {
+ test('becomes visible when focused (top: 0)', async () => {
+ render(<SkipLink />);
+ const link = screen.getByRole('link');
+ link.focus();
+ expect(link).toHaveStyleRule('top', '0 !important', {
+ modifier: ':focus',
+ });
+ });
+
+ test('receives focus on Tab key press', async () => {
+ const user = userEvent.setup();
+ render(<SkipLink />);
+ await user.tab();
+ const link = screen.getByRole('link');
+ expect(link).toHaveFocus();
+ });
+
+ test('shows focus outline styling', () => {
+ render(<SkipLink />);
+ const link = screen.getByRole('link');
+ expect(link).toHaveStyleRule('outline', expect.stringContaining('3px
solid'), {
+ modifier: ':focus',
+ });
+ });
+
+ test('shows focus-visible outline styling', () => {
+ render(<SkipLink />);
+ const link = screen.getByRole('link');
+ expect(link).toHaveStyleRule('top', '0 !important', {
+ modifier: ':focus-visible',
+ });
+ });
+ });
+
+ describe('Keyboard Navigation', () => {
+ test('activates on Enter key press', async () => {
+ const user = userEvent.setup();
+ const targetElement = document.createElement('main');
+ targetElement.id = 'main-content';
+ document.body.appendChild(targetElement);
+
+ render(<SkipLink />);
+ await user.tab();
+ await user.keyboard('{Enter}');
+
+ await waitFor(() => {
+ expect(targetElement).toHaveFocus();
+ });
+
+ targetElement.remove();
+ });
+
+ test('activates on click', async () => {
+ const user = userEvent.setup();
+ const targetElement = document.createElement('main');
+ targetElement.id = 'main-content';
+ document.body.appendChild(targetElement);
+
+ render(<SkipLink />);
+ await user.click(screen.getByRole('link'));
+
+ await waitFor(() => {
+ expect(targetElement).toHaveFocus();
+ });
+
+ targetElement.remove();
+ });
+ });
+
+ describe('Click Handler', () => {
+ test('prevents default anchor behavior', async () => {
+ const user = userEvent.setup();
Review Comment:
Unused variable user.
```suggestion
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -107,63 +111,55 @@ const ActionButtons = ({
dataMaskSelected,
isApplyDisabled,
filterBarOrientation = FilterBarOrientation.Vertical,
- chartCustomizationItems,
}: ActionButtonsProps) => {
const isClearAllEnabled = useMemo(() => {
- const hasSelectedChanges = Object.entries(dataMaskSelected).some(
- ([, mask]) => {
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- },
+ // Check both selected (pending) and applied filters to determine if clear
is available
+ const hasSelectedValues = Object.values(dataMaskSelected).some(
+ mask => isDefined(mask.filterState?.value),
);
-
- const hasAppliedChanges = Object.entries(dataMaskApplied).some(
- ([, mask]) => {
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- },
+ const hasAppliedValues = Object.values(dataMaskApplied).some(
+ mask => isDefined(mask.filterState?.value),
);
-
- const hasChartCustomizations = chartCustomizationItems?.some(item => {
- if (item.removed) return false;
- const mask = dataMaskApplied[item.id] || dataMaskSelected[item.id];
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- });
-
- return hasSelectedChanges || hasAppliedChanges || hasChartCustomizations;
- }, [dataMaskSelected, dataMaskApplied, chartCustomizationItems]);
+ return hasSelectedValues || hasAppliedValues;
+ }, [dataMaskSelected, dataMaskApplied]);
Review Comment:
The isClearAllEnabled logic has been simplified and now only checks for
`filterState?.value`, but the original implementation also checked for
`ownState?.column` (hasGroupBy). This change may cause the "Clear all" button
to be incorrectly disabled when filters have groupBy columns but no explicit
values. This could break existing filter clearing functionality.
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -165,200 +171,147 @@ const SliceHeader = forwardRef<HTMLDivElement,
SliceHeaderProps>(
formData,
width,
height,
- exportPivotExcel = () => ({}),
},
ref,
) => {
- const SliceHeaderExtension = extensionsRegistry.get(
- 'dashboard.slice.header',
- );
- const uiConfig = useUiConfig();
- const shouldShowRowLimitWarning =
- !isEmbedded() || uiConfig.showRowLimitWarning;
- const dashboardPageId = useContext(DashboardPageIdContext);
- const [headerTooltip, setHeaderTooltip] = useState<ReactNode | null>(null);
- const headerRef = useRef<HTMLDivElement>(null);
- // TODO: change to indicator field after it will be implemented
- const crossFilterValue = useSelector<RootState, any>(
- state => state.dataMask[slice?.slice_id]?.filterState?.value,
- );
- const isCrossFiltersEnabled = useSelector<RootState, boolean>(
- ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
- );
-
- const firstQueryResponse = useSelector<RootState, QueryData | undefined>(
- state => state.charts[slice.slice_id].queriesResponse?.[0],
- );
-
- const theme = useTheme();
-
- const rowLimit = Number(formData.row_limit || -1);
- const sqlRowCount = Number(firstQueryResponse?.sql_rowcount || 0);
+ const SliceHeaderExtension =
extensionsRegistry.get('dashboard.slice.header');
+ const uiConfig = useUiConfig();
+ const dashboardPageId = useContext(DashboardPageIdContext);
+ const [headerTooltip, setHeaderTooltip] = useState<ReactNode | null>(null);
+ const headerRef = useRef<HTMLDivElement>(null);
+ // TODO: change to indicator field after it will be implemented
+ const crossFilterValue = useSelector<RootState, any>(
+ state => state.dataMask[slice?.slice_id]?.filterState?.value,
+ );
+ const isCrossFiltersEnabled = useSelector<RootState, boolean>(
+ ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
+ );
- const canExplore = !editMode && supersetCanExplore;
+ const canExplore = !editMode && supersetCanExplore;
- useEffect(() => {
- const headerElement = headerRef.current;
- if (canExplore) {
- setHeaderTooltip(getSliceHeaderTooltip(sliceName));
- } else if (
- headerElement &&
- (headerElement.scrollWidth > headerElement.offsetWidth ||
- headerElement.scrollHeight > headerElement.offsetHeight)
- ) {
- setHeaderTooltip(sliceName ?? null);
- } else {
- setHeaderTooltip(null);
- }
- }, [sliceName, width, height, canExplore]);
-
- const exploreUrl =
`/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${slice.slice_id}`;
+ useEffect(() => {
+ const headerElement = headerRef.current;
+ if (canExplore) {
+ setHeaderTooltip(getSliceHeaderTooltip(sliceName));
+ } else if (
+ headerElement &&
+ (headerElement.scrollWidth > headerElement.offsetWidth ||
+ headerElement.scrollHeight > headerElement.offsetHeight)
+ ) {
+ setHeaderTooltip(sliceName ?? null);
+ } else {
+ setHeaderTooltip(null);
+ }
+ }, [sliceName, width, height, canExplore]);
- const renderExploreLink = (title: string) => (
- <Link
- to={exploreUrl}
- css={(theme: SupersetTheme) => css`
- color: ${theme.colorText};
- text-decoration: none;
- :hover {
- text-decoration: underline;
- }
- display: inline-block;
- `}
- >
- {title}
- </Link>
- );
+ const exploreUrl =
`/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${slice.slice_id}`;
- return (
- <ChartHeaderStyles data-test="slice-header" ref={ref}>
- <div className="header-title" ref={headerRef}>
+ return (
+ <ChartHeaderStyles data-test="slice-header" ref={ref}>
+ <div className="header-title" ref={headerRef}>
+ {/* WCAG 1.3.1: Chart title as semantic heading for screen readers */}
+ <h2>
<Tooltip title={headerTooltip}>
- {/* this div ensures the hover event triggers correctly and
prevents flickering */}
- <div>
- <EditableTitle
- title={
- sliceName ||
- (editMode
- ? '---' // this makes an empty title clickable
- : '')
- }
- canEdit={editMode}
- onSaveTitle={updateSliceName}
- showTooltip={false}
- renderLink={
- canExplore && exploreUrl ? renderExploreLink : undefined
- }
- />
- </div>
+ <EditableTitle
+ title={
+ sliceName ||
+ (editMode
+ ? '---' // this makes an empty title clickable
+ : '')
+ }
+ canEdit={editMode}
+ onSaveTitle={updateSliceName}
+ showTooltip={false}
+ url={canExplore ? exploreUrl : undefined}
Review Comment:
The EditableTitle component does not accept a `url` prop. According to the
component's API, you should use the `renderLink` callback prop instead, which
takes the title as a parameter and returns a React node. The current usage will
cause TypeScript errors or runtime issues.
You need to pass a function that renders a link component rather than just a
URL string.
```suggestion
renderLink={
canExplore && exploreUrl
? title => <a href={exploreUrl}>{title}</a>
: undefined
}
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -107,63 +111,55 @@ const ActionButtons = ({
dataMaskSelected,
isApplyDisabled,
filterBarOrientation = FilterBarOrientation.Vertical,
- chartCustomizationItems,
}: ActionButtonsProps) => {
const isClearAllEnabled = useMemo(() => {
- const hasSelectedChanges = Object.entries(dataMaskSelected).some(
- ([, mask]) => {
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- },
+ // Check both selected (pending) and applied filters to determine if clear
is available
+ const hasSelectedValues = Object.values(dataMaskSelected).some(
+ mask => isDefined(mask.filterState?.value),
);
-
- const hasAppliedChanges = Object.entries(dataMaskApplied).some(
- ([, mask]) => {
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- },
+ const hasAppliedValues = Object.values(dataMaskApplied).some(
+ mask => isDefined(mask.filterState?.value),
);
-
- const hasChartCustomizations = chartCustomizationItems?.some(item => {
- if (item.removed) return false;
- const mask = dataMaskApplied[item.id] || dataMaskSelected[item.id];
- const hasValue = isDefined(mask?.filterState?.value);
- const hasGroupBy = isDefined(mask?.ownState?.column);
- return hasValue || hasGroupBy;
- });
-
- return hasSelectedChanges || hasAppliedChanges || hasChartCustomizations;
- }, [dataMaskSelected, dataMaskApplied, chartCustomizationItems]);
+ return hasSelectedValues || hasAppliedValues;
+ }, [dataMaskSelected, dataMaskApplied]);
const isVertical = filterBarOrientation === FilterBarOrientation.Vertical;
return (
- <ButtonsContainer
- isVertical={isVertical}
- width={width}
+ <div
+ css={(theme: SupersetTheme) => [
+ containerStyle(theme),
+ isVertical ? verticalStyle(theme, width) : horizontalStyle(theme),
+ ]}
data-test="filterbar-action-buttons"
>
- <Button
- disabled={isApplyDisabled}
- buttonStyle="primary"
- htmlType="submit"
- className="filter-apply-button"
- onClick={onApply}
- {...getFilterBarTestId('apply-button')}
+ <Tooltip
+ title={isApplyDisabled ? t('Select filter values to apply') :
undefined}
+ placement="top"
>
- {isVertical ? t('Apply filters') : t('Apply')}
- </Button>
+ <span>
+ <Button
+ disabled={isApplyDisabled}
+ buttonStyle="primary"
+ htmlType="submit"
+ className="filter-apply-button"
+ onClick={onApply}
+ {...getFilterBarTestId('apply-button')}
+ >
+ {isVertical ? t('Apply filters') : t('Apply')}
+ </Button>
+ </span>
Review Comment:
Wrapping the Button in a span element to support the Tooltip is correct for
disabled buttons (since disabled buttons don't trigger mouse events). However,
the span should have `display: inline-block` styling to ensure proper tooltip
positioning and behavior. Without this, the tooltip may not appear correctly in
all scenarios.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
