Aitema-gmbh commented on code in PR #39241:
URL: https://github.com/apache/superset/pull/39241#discussion_r3232686709
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -649,6 +664,7 @@ const DashboardBuilder = () => {
{renderDraggableContent}
</Droppable>
</StyledHeader>
+ <SrOnlyH2>{t('Dashboard content')}</SrOnlyH2>
Review Comment:
ADDRESSED: Follow-up commit 2679d1a added a sibling `<VisuallyHidden
as="h2">{t('Filters')}</VisuallyHidden>` for the horizontal filter bar
(DashboardBuilder.tsx L515). Both orientations now have an equivalent sr-only
section heading.
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -616,6 +628,8 @@ const DashboardBuilder = () => {
return (
<DashboardWrapper>
{isVerticalFilterBarVisible && (
+ <>
+ <SrOnlyH2>{t('Filters')}</SrOnlyH2>
<ResizableSidebar
Review Comment:
ADDRESSED: Same fix as the sibling thread — commit 2679d1a renders a
`VisuallyHidden as="h2"` heading in the horizontal branch as well, so WCAG
2.4.6 coverage is consistent across both filter-bar orientations.
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:
##########
@@ -84,6 +84,18 @@ export interface ExploreChartHeaderProps {
isSaveModalVisible?: boolean;
}
+const SrOnlyH1 = styled.h1`
+ position: absolute;
+ width: 1px;
+ height: 1px;
+ padding: 0;
+ margin: -1px;
Review Comment:
ADDRESSED: Commit 2679d1a extracts a shared `VisuallyHidden` component under
`src/components/Accessibility/` (with an `as` prop) and replaces every per-file
SrOnlyH1/SrOnlyH2 block — ExploreChartHeader included. No copy-pasted sr-only
CSS remains in src.
##########
superset-frontend/src/dashboard/components/Header/index.tsx:
##########
@@ -177,6 +177,18 @@ const actionButtonsStyle = (theme: SupersetTheme) => css`
}
`;
+const SrOnlyH1 = styled.h1`
+ position: absolute;
+ width: 1px;
+ height: 1px;
+ padding: 0;
Review Comment:
ADDRESSED: Same fix — commit 2679d1a migrates Header/index.tsx to the shared
`VisuallyHidden` component from `src/components/Accessibility/`. No duplicated
sr-only CSS remains.
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -509,10 +521,13 @@ const DashboardBuilder = () => {
{!hideDashboardHeader && <DashboardHeader />}
{showFilterBar &&
filterBarOrientation === FilterBarOrientation.Horizontal && (
- <FilterBar
- orientation={FilterBarOrientation.Horizontal}
- hidden={isReport}
- />
+ <>
+ <SrOnlyH2>{t('Filters')}</SrOnlyH2>
Review Comment:
ADDRESSED (with note): The `hidden={isReport}` path is print/PDF-report
rendering where the filter bar is intentionally suppressed and no interactive
screen-reader session occurs — a stray sr-only heading there has no practical
user impact. The duplicate-heading bug you flagged in the vertical branch
(separate thread) was the real issue and is fixed in 2679d1a. Happy to gate the
heading on `!isReport` as a follow-up if reviewers prefer.
--
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]
