rusackas commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652260708
##########
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:
Native `<button>` already handles Enter/Space without a custom handler, same
as every other role-to-tag swap in this PR. Adding a keyboard test just for
this one spot would be inconsistent with the rest, so leaving it be.
##########
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:
Same story as the rest of these conversions, native activation instead of
re-adding the repeat guard. Would be inconsistent to special-case just this one.
##########
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:
This rule's real, it's oxlint's jsx-a11y(prefer-tag-over-role), defined in
oxlint.json which is what actually lints this repo (eslint proper isn't run
here at all). Ran oxlint over the file and the disable comment suppresses it
clean.
--
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]
