bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652260900
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx:
##########
@@ -191,19 +190,25 @@ const FilterTitleContainer = forwardRef<HTMLDivElement,
Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
Review Comment:
<!-- Bito Reply -->
The suggestion to add a keyboard interaction test is intended to ensure that
the new button remains accessible, as native button behavior can sometimes be
obscured or overridden by custom styling or component wrappers. While native
elements handle keyboard events by default, explicitly testing this interaction
confirms that the component's implementation does not interfere with standard
browser behavior. If the project's testing strategy consistently avoids
redundant tests for native elements, it is reasonable to omit this test to
maintain consistency.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx:
##########
@@ -159,19 +158,25 @@ const ItemTitleContainer = forwardRef<HTMLDivElement,
Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
data-test="undo-button"
- tabIndex={0}
onClick={e => {
e.preventDefault();
restoreItem(id);
}}
- onKeyDown={handleKeyboardActivation(() => restoreItem(id))}
>
Review Comment:
<!-- Bito Reply -->
The suggestion is to re-add a keyboard repeat handler to the new `<button>`
element to prevent duplicate `restoreItem` calls when the Enter or Space key is
held down. Applying this suggestion is appropriate because it maintains the
original behavior of ignoring auto-repeat keydown events, which is necessary
for consistent interaction handling.
**superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx**
```
onClick={e => {
e.preventDefault();
restoreItem(id);
}}
onKeyDown={handleKeyboardActivation(() => restoreItem(id))}
```
##########
superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.test.tsx:
##########
@@ -26,6 +26,7 @@ jest.mock(
'@superset-ui/core/components/Icons/AsyncIcon',
() =>
({ fileName }: { fileName: string }) => (
+ // eslint-disable-next-line jsx-a11y/prefer-tag-over-role -- mirrors
AsyncIcon's real span+role="img" shape
<span role="img" aria-label={fileName.replace('_', '-')} />
),
Review Comment:
<!-- Bito Reply -->
If the rule is not part of the project's ESLint configuration and is instead
managed by oxlint, the suggestion to remove the disable comment is not
applicable. You should ignore this suggestion, as the comment is necessary for
the project's actual linting setup.
--
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]
