Copilot commented on code in PR #34330:
URL: https://github.com/apache/superset/pull/34330#discussion_r2241016579
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -168,6 +191,38 @@ export const getColorFunction = (
? { cutoffValue: targetValueLeft!, extremeValue: targetValueRight! }
: false;
break;
+ case Comparator.BeginsWith:
+ comparatorFunction = (value: string) =>
+ typeof value === 'string' &&
+ value?.startsWith(targetValue as unknown as string)
+ ? { cutoffValue: targetValue!, extremeValue: targetValue! }
+ : false;
+ break;
+ case Comparator.EndsWith:
+ comparatorFunction = (value: string) =>
+ typeof value === 'string' &&
+ value?.endsWith(targetValue as unknown as string)
+ ? { cutoffValue: targetValue!, extremeValue: targetValue! }
+ : false;
+ break;
+ case Comparator.Containing:
+ comparatorFunction = (value: string) =>
+ typeof value === 'string' &&
+ value
+ ?.toLowerCase()
+ .includes((targetValue as unknown as string).toLowerCase())
+ ? { cutoffValue: targetValue!, extremeValue: targetValue! }
+ : false;
+ break;
+ case Comparator.NotContaining:
+ comparatorFunction = (value: string) =>
+ typeof value === 'string' &&
+ !value
+ ?.toLowerCase()
+ .includes((targetValue as unknown as string).toLowerCase())
Review Comment:
The type assertion `as unknown as string` is used multiple times and
suggests potential type safety issues. Consider updating the type definitions
to properly handle string values or add proper type guards.
```suggestion
isString(targetValue) &&
value?.startsWith(targetValue)
? { cutoffValue: targetValue!, extremeValue: targetValue! }
: false;
break;
case Comparator.EndsWith:
comparatorFunction = (value: string) =>
typeof value === 'string' &&
isString(targetValue) &&
value?.endsWith(targetValue)
? { cutoffValue: targetValue!, extremeValue: targetValue! }
: false;
break;
case Comparator.Containing:
comparatorFunction = (value: string) =>
typeof value === 'string' &&
isString(targetValue) &&
value
?.toLowerCase()
.includes(targetValue.toLowerCase())
? { cutoffValue: targetValue!, extremeValue: targetValue! }
: false;
break;
case Comparator.NotContaining:
comparatorFunction = (value: string) =>
typeof value === 'string' &&
isString(targetValue) &&
!value
?.toLowerCase()
.includes(targetValue.toLowerCase())
```
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -253,10 +335,12 @@ export const FormattingPopoverContent = ({
</Row>
<FormItem noStyle shouldUpdate={shouldFormItemUpdate}>
{showOperatorFields ? (
- renderOperatorFields
+ (props: GetFieldValue) => renderOperatorFields(props, columnType)
) : (
- <Row gutter={12}>
- <Col span={6}>{renderOperator({ showOnlyNone: true })}</Col>
+ <Row style={{ display: 'none' }} gutter={12}>
+ <Col span={6}>
+ {renderOperator({ showOnlyNone: true, columnType })}
+ </Col>
Review Comment:
[nitpick] Using inline styles with `display: 'none'` is not ideal for
maintainability. Consider using a conditional render or CSS class instead.
```suggestion
<Row gutter={12}>
{showOperatorFields && (
<Col span={6}>
{renderOperator({ showOnlyNone: true, columnType })}
</Col>
)}
```
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -168,6 +191,38 @@ export const getColorFunction = (
? { cutoffValue: targetValueLeft!, extremeValue: targetValueRight! }
: false;
break;
+ case Comparator.BeginsWith:
+ comparatorFunction = (value: string) =>
+ typeof value === 'string' &&
+ value?.startsWith(targetValue as unknown as string)
+ ? { cutoffValue: targetValue!, extremeValue: targetValue! }
+ : false;
+ break;
+ case Comparator.EndsWith:
+ comparatorFunction = (value: string) =>
+ typeof value === 'string' &&
+ value?.endsWith(targetValue as unknown as string)
+ ? { cutoffValue: targetValue!, extremeValue: targetValue! }
+ : false;
+ break;
+ case Comparator.Containing:
+ comparatorFunction = (value: string) =>
+ typeof value === 'string' &&
Review Comment:
[nitpick] The type checking `typeof value === 'string'` is repeated across
multiple string comparison cases. Consider extracting this into a helper
function or consolidating the logic to reduce duplication.
```suggestion
isString(value) &&
value?.startsWith(targetValue as unknown as string)
? { cutoffValue: targetValue!, extremeValue: targetValue! }
: false;
break;
case Comparator.EndsWith:
comparatorFunction = (value: string) =>
isString(value) &&
value?.endsWith(targetValue as unknown as string)
? { cutoffValue: targetValue!, extremeValue: targetValue! }
: false;
break;
case Comparator.Containing:
comparatorFunction = (value: string) =>
isString(value) &&
```
--
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]
