bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3588320494
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -35,14 +39,26 @@ export const ActionButton = ({
placement,
icon,
onClick,
+ className,
+ disabled = false,
+ dataTest,
}: ActionProps) => {
const theme = useTheme();
const actionButton = (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ aria-disabled={disabled}
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to add `tabIndex={-1}` to a disabled button is a
common accessibility pattern, but your approach of using
`aria-disabled={disabled}` on a native `<button>` element is also a valid and
often preferred pattern. When using a native `<button>` with the `disabled`
attribute, the browser automatically handles focus and accessibility. If you
are using `aria-disabled` to keep the button focusable while indicating its
state to screen readers, adding `tabIndex={-1}` would indeed prevent keyboard
users from focusing the element, which contradicts your goal of keeping it
focusable.
**superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx**
```
+ <button
+ type="button"
+ aria-disabled={disabled}
```
##########
superset-frontend/src/features/charts/ChartCard.tsx:
##########
@@ -103,9 +115,9 @@ export default function ChartCard({
menuItems.push({
key: 'export',
label: (
- <div
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={menuItemButtonCss}
Review Comment:
<!-- Bito Reply -->
The suggestion to add `data-test="chart-list-export-option"` is appropriate
for maintaining consistency with other action buttons in the component, such as
edit and delete. Applying this attribute ensures that automated tests can
reliably target the export action in the card view, mirroring the pattern used
elsewhere in the codebase.
**superset-frontend/src/features/charts/ChartCard.tsx**
```
+ <button
+ type="button"
+ css={menuItemButtonCss}
+ data-test="chart-list-export-option"
```
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -52,12 +68,12 @@ export const ActionButton = ({
}
}
Review Comment:
<!-- Bito Reply -->
The suggestion to add visual feedback for the disabled state is appropriate
and improves the user experience by providing clear cues. Applying this change
is recommended to ensure consistency with the existing design patterns in the
codebase.
--
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]
