bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652261313
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControlShared.tsx:
##########
@@ -236,6 +236,10 @@ export const DescriptionToolTip = ({
whiteSpace: 'normal',
}}
>
+ {/* Deliberate role="button" on this tooltip-trigger icon (no click
+ handler) — covered by existing test expectations; not a fit for
+ the linter's suggested <button> tag. */}
Review Comment:
<!-- Bito Reply -->
The adjustment to the comment is appropriate. By removing the claim of test
coverage where none exists and referencing the correct pattern, the
documentation is now accurate and prevents misleading future developers.
**superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControlShared.tsx**
```
+ {/* Deliberate role="button" on this tooltip-trigger icon (no click
+ handler) — pattern follows DeckglLayerVisibilityTooltip; not a fit
+ for the linter's suggested <button> tag. */}
```
##########
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx:
##########
@@ -344,29 +349,20 @@ const Thumbnail: FC<ThumbnailProps> = ({
const { key, value: type } = entry;
const isSelected = selectedViz === entry.key;
- const handleKeyDown = (event: React.KeyboardEvent) => {
- if (event.key === 'Enter' || event.key === ' ') {
- event.preventDefault();
- setSelectedViz(key);
- }
- };
-
const handleFocus = () => {
// Auto-select chart when tabbed to
setSelectedViz(key);
};
return (
- <div
- role="button"
+ <button
+ type="button"
// using css instead of a styled component to preserve
// the data-test attribute
css={thumbnailContainerCss(theme)}
- tabIndex={0}
className={isSelected ? 'selected' : ''}
onClick={() => setSelectedViz(key)}
onDoubleClick={onDoubleClick}
- onKeyDown={handleKeyDown}
onFocus={handleFocus}
data-test="viztype-selector-container"
Review Comment:
<!-- Bito Reply -->
The addition of `aria-pressed={isSelected}` to the button element is correct
and addresses the accessibility requirement for toggle buttons. This attribute
properly communicates the selected state to assistive technologies, ensuring
compliance with accessibility standards.
**superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx**
```
<button
type="button"
aria-pressed={isSelected}
// using css instead of a styled component to preserve
// the data-test attribute
css={thumbnailContainerCss(theme)}
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx:
##########
@@ -104,24 +110,13 @@ const UrlFiltersVerticalCollapse = (props: {
return (
<div css={sectionContainerStyle}>
- <div
- css={sectionHeaderStyle}
- onClick={toggleSection}
- onKeyDown={e => {
- if (e.key === 'Enter' || e.key === ' ') {
- e.preventDefault();
- toggleSection();
- }
- }}
- role="button"
- tabIndex={0}
- >
+ <button type="button" css={sectionHeaderStyle} onClick={toggleSection}>
Review Comment:
<!-- Bito Reply -->
The suggestion to add `aria-expanded` is appropriate for improving
accessibility. By binding `aria-expanded` to the `sectionsOpen` state, screen
reader users will receive clear feedback on whether the section is currently
expanded or collapsed, which is a standard best practice for toggleable UI
components.
**superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx**
```
<button
type="button"
css={sectionHeaderStyle}
onClick={toggleSection}
aria-expanded={isOpen}>
```
--
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]
