bito-code-review[bot] commented on code in PR #41132:
URL: https://github.com/apache/superset/pull/41132#discussion_r3470078878
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.tsx:
##########
@@ -346,9 +346,9 @@ class AdhocMetricEditPopover extends PureComponent<
savedMetric: propsSavedMetric,
columns,
savedMetricsOptions,
- onChange,
- onClose,
- onResize,
+ onChange: _onChange,
+ onClose: _onClose,
+ onResize: _onResize,
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to remove the destructuring aliases is based on
the observation that they appear unused in the component. However, your
explanation clarifies that these aliases are intentionally used to prevent
`onChange`, `onClose`, and `onResize` from being spread into `popoverProps` and
subsequently passed to a child element. Given this functional requirement to
exclude these props from the child, the current implementation is correct and
the suggestion should not be applied.
##########
superset-frontend/packages/superset-ui-core/src/components/Label/index.tsx:
##########
@@ -34,7 +34,7 @@ export const Label = forwardRef<HTMLSpanElement,
LabelProps>((props, ref) => {
onClick,
children,
icon,
- id,
+ id: _id,
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to revert to `id` is based on the assumption that
`id` should be passed through to the `<Tag>` component via `...rest`. However,
as you noted, `id` was already being destructured separately and was not part
of the `...rest` object. Therefore, the current implementation of renaming `id`
to `_id` effectively silences the lint rule without altering the component's
existing behavior regarding prop forwarding. If the intention is to maintain
the current behavior, the rename is acceptable; if the intention is to actually
expose the `id` prop to the underlying component, then reverting to `id` would
be necessary.
**superset-frontend/packages/superset-ui-core/src/components/Label/index.tsx**
```
34: onClick,
35: children,
36: icon,
37: - id: _id,
37: + id,
38: ...rest
39: } = props;
```
##########
superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx:
##########
@@ -66,7 +66,7 @@ function findMatchingTimezone(
export default function TimezoneSelector({
onTimezoneChange,
timezone,
- minWidth = MIN_SELECT_WIDTH,
+ minWidth: _minWidth = MIN_SELECT_WIDTH,
Review Comment:
<!-- Bito Reply -->
The suggestion to either remove the unused parameter or use it in the
component is valid. While renaming the prop to `_minWidth` satisfies the
linting rule, it does not address the underlying issue of the prop being
silently discarded. If you prefer to keep the prop for future use, you could
wire it into the `<Select>` component as suggested by the reviewer.
**superset-frontend/packages/superset-ui-core/src/components/TimezoneSelector/index.tsx**
```
export default function TimezoneSelector({
onTimezoneChange,
timezone,
minWidth: _minWidth = MIN_SELECT_WIDTH,
```
--
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]
