bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3588318510
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -184,10 +184,15 @@ const ViewModeContainer = styled.div`
display: inline-block;
.toggle-button {
+ appearance: none;
+ border: none;
+ background: none;
+ font: inherit;
display: inline-block;
border-radius: ${theme.borderRadius}px;
padding: ${theme.sizeUnit}px;
padding-bottom: ${theme.sizeUnit * 0.5}px;
+ cursor: pointer;
Review Comment:
<!-- Bito Reply -->
The observation is correct. Since the template string in `.toggle-button` is
not tagged with `css`, the interpolated `inlineTextButtonCss` would be treated
as a plain string rather than being processed by the emotion serializer.
Maintaining the duplication is the appropriate approach in this specific
context to ensure the styles are applied correctly.
**superset-frontend/src/components/ListView/ListView.tsx**
```
.toggle-button {
+ appearance: none;
+ border: none;
+ background: none;
+ font: inherit;
display: inline-block;
```
##########
superset-frontend/src/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
Review Comment:
<!-- Bito Reply -->
The suggestion from the reviewer is based on the assumption that the
`data-test` attribute on the icon is required for testing purposes and that
moving it inside the `icon` prop of `ActionButton` makes it inaccessible to
tests. If your existing tests successfully query the icon element directly
using `data-test="dashboard-list-trash-icon"` even after this change, the
suggestion may be a false positive in your specific testing environment. You
can safely ignore this suggestion if your test suite remains passing.
**superset-frontend/src/pages/Tags/index.tsx**
```
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
```
##########
superset-frontend/src/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
)}
</ConfirmStatusChange>
)}
{canEdit && (
- <Tooltip
- id="edit-action-tooltip"
- title={t('Edit')}
+ <ActionButton
+ label={t('Edit')}
+ tooltip={t('Edit')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={handleEdit}
- >
+ icon={
<Icons.EditOutlined data-test="edit-alt" iconSize="l" />
- </span>
- </Tooltip>
+ }
+ onClick={handleEdit}
+ />
)}
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion is correct. While the `data-test` attribute on the
icon remains, the `ActionButton` component defaults its `data-test` attribute
to the `label` prop when `dataTest` is not explicitly provided. To ensure
existing tests that query by `edit-alt` continue to pass, you should pass
`dataTest="edit-alt"` to the `ActionButton` component.
**superset-frontend/src/pages/Tags/index.tsx**
```
<ActionButton
label={t('Edit')}
tooltip={t('Edit')}
placement="bottom"
dataTest="edit-alt"
icon={
<Icons.EditOutlined data-test="edit-alt" iconSize="l" />
}
onClick={handleEdit}
/>
```
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx:
##########
@@ -113,24 +113,30 @@ const AdhocMetricEditPopoverTitle:
FC<AdhocMetricEditPopoverTitleProps> = ({
return (
<Tooltip placement="top" title={t('Click to edit label')}>
- <span
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
Review Comment:
<!-- Bito Reply -->
The suggestion to add `cursor: pointer` to the button is appropriate. Since
the element was converted from a `<span>` to a `<button>`, explicitly setting
`cursor: pointer` ensures consistent interactive behavior, matching other
similar components in the codebase.
**superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx**
```
css={css`
appearance: none;
border: none;
background: none;
padding: 0;
font: inherit;
cursor: pointer;
`}
```
--
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]
