bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3592910331
##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -291,17 +286,21 @@ export default function DataSourcePanel({
message=""
description={
<>
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
onClick={() => setShowSaveDatasetModal(true)}
- onKeyDown={handleKeyboardActivation(() =>
- setShowSaveDatasetModal(true),
- )}
className="add-dataset-alert-description"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ cursor: pointer;
+ `}
Review Comment:
<!-- Bito Reply -->
You are correct that the CSS provided does not explicitly set an `outline`
property, which allows the browser's default focus ring to remain visible for
keyboard users. If you wish to customize or remove this focus style, you can
add an `outline` property to the `css` block, such as `outline: none;` to
remove it or a custom style like `outline: 2px solid blue;` for better
visibility.
**superset-frontend/src/explore/components/DatasourcePanel/index.tsx**
```
css={css`
appearance: none;
border: none;
background: none;
padding: 0;
font: inherit;
cursor: pointer;
outline: none;
`}
```
##########
superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx:
##########
@@ -184,30 +180,31 @@ export const DataTablesPane = ({
) : (
<Icons.DownOutlined aria-label={t('Expand data panel')} />
);
+ const resetButtonCss = css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ `;
return (
<div>
{panelOpen ? (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(false)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(false),
- )}
>
{caretIcon}
- </span>
+ </button>
) : (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(true)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(true),
- )}
>
{caretIcon}
- </span>
+ </button>
Review Comment:
<!-- Bito Reply -->
The change effectively addresses the suggestion by consolidating the two
conditional buttons into a single button element. This reduces code duplication
and simplifies the component structure while maintaining the required
functionality.
**superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx**
```
return (
<div>
{panelOpen ? (
<button
type="button"
css={resetButtonCss}
onClick={() => handleCollapseChange(false)}
>
{caretIcon}
</button>
) : (
<button
type="button"
css={resetButtonCss}
onClick={() => handleCollapseChange(true)}
>
{caretIcon}
</button>
```
--
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]
