You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "SBIN2010 (via GitHub)" <gi...@apache.org> on 2025/11/13 14:41:30 UTC
[PR] fix: opacity color formating [superset]
SBIN2010 opened a new pull request, #36101:
URL: https://github.com/apache/superset/pull/36101
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR fix #36098
Fixed data type checking which caused opacity to be set to 1
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
<img width="2202" height="917" alt="Снимок экрана от 2025-11-13 17-37-22" src="https://github.com/user-attachments/assets/b4d0908b-4c9e-4a57-acc9-a39293aa6262" />
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1)Create a table chart type
2)Add a custom conditional formatting - you can see the bug by using > x or value < x < value
3)Select "alert" or "success" or "error" for the color scheme
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #36101:
URL: https://github.com/apache/superset/pull/36101#discussion_r2525118514
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -38,20 +38,20 @@ export const getOpacity = (
minOpacity = MIN_OPACITY_BOUNDED,
maxOpacity = MAX_OPACITY,
) => {
- if (
- extremeValue === cutoffPoint ||
- typeof cutoffPoint !== 'number' ||
- typeof extremeValue !== 'number' ||
- typeof value !== 'number'
- ) {
+ if (extremeValue === cutoffPoint || typeof value !== 'number') {
return maxOpacity;
}
+ const numCutoffPoint =
+ typeof cutoffPoint === 'string' ? parseFloat(cutoffPoint) : cutoffPoint;
+ const numExtremeValue =
+ typeof extremeValue === 'string' ? parseFloat(extremeValue) : extremeValue;
Review Comment:
Should we guard in case if `numCutoffPoint` or `numExtremeValue` doesn't return a number? I see we check for a string but what about `NaN`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36101:
URL: https://github.com/apache/superset/pull/36101#issuecomment-3532602298
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19365192267) for [18a71bb](https://github.com/apache/superset/commit/18a71bb)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #36101:
URL: https://github.com/apache/superset/pull/36101#issuecomment-3530011153
@SBIN2010 I know this is a fairly small change, but can we add some tests to make sure opacity returned correctly for different value types? Like if we use string and combo of string and number? Ok to move this to do a follow up pr if you want.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 merged PR #36101:
URL: https://github.com/apache/superset/pull/36101
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on code in PR #36101:
URL: https://github.com/apache/superset/pull/36101#discussion_r2525156296
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -38,20 +38,20 @@ export const getOpacity = (
minOpacity = MIN_OPACITY_BOUNDED,
maxOpacity = MAX_OPACITY,
) => {
- if (
- extremeValue === cutoffPoint ||
- typeof cutoffPoint !== 'number' ||
- typeof extremeValue !== 'number' ||
- typeof value !== 'number'
- ) {
+ if (extremeValue === cutoffPoint || typeof value !== 'number') {
return maxOpacity;
}
+ const numCutoffPoint =
+ typeof cutoffPoint === 'string' ? parseFloat(cutoffPoint) : cutoffPoint;
+ const numExtremeValue =
+ typeof extremeValue === 'string' ? parseFloat(extremeValue) : extremeValue;
Review Comment:
Added a check for Nan
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36101:
URL: https://github.com/apache/superset/pull/36101#issuecomment-3530291873
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19350134865) for [665c6be](https://github.com/apache/superset/commit/665c6be)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36101:
URL: https://github.com/apache/superset/pull/36101#issuecomment-3532631931
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19365192267) for [18a71bb](https://github.com/apache/superset/commit/18a71bb)
• **Environment:** http://44.251.141.119:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36101:
URL: https://github.com/apache/superset/pull/36101#issuecomment-3530326626
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19350134865) for [665c6be](https://github.com/apache/superset/commit/665c6be)
• **Environment:** http://34.215.83.252:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #36101:
URL: https://github.com/apache/superset/pull/36101#discussion_r2525154809
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -38,20 +38,20 @@ export const getOpacity = (
minOpacity = MIN_OPACITY_BOUNDED,
maxOpacity = MAX_OPACITY,
) => {
- if (
- extremeValue === cutoffPoint ||
- typeof cutoffPoint !== 'number' ||
- typeof extremeValue !== 'number' ||
- typeof value !== 'number'
- ) {
+ if (extremeValue === cutoffPoint || typeof value !== 'number') {
return maxOpacity;
}
+ const numCutoffPoint =
+ typeof cutoffPoint === 'string' ? parseFloat(cutoffPoint) : cutoffPoint;
+ const numExtremeValue =
+ typeof extremeValue === 'string' ? parseFloat(extremeValue) : extremeValue;
Review Comment:
Looks like Copilot had a similar comment and was marked resolved. Can we add tests for these cases to ensure maxOpacity is still returned correctly if tests don't already exist?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #36101:
URL: https://github.com/apache/superset/pull/36101#discussion_r2524514971
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -256,6 +256,8 @@ export const getColorFormatters = memoizeOne(
resolvedColorScheme = theme[config.colorScheme] as string;
}
+ console.log('resolvedColorScheme', resolvedColorScheme);
Review Comment:
Debug console.log statement should be removed before merging to production. This will clutter the browser console unnecessarily.
```suggestion
```
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -38,20 +38,20 @@ export const getOpacity = (
minOpacity = MIN_OPACITY_BOUNDED,
maxOpacity = MAX_OPACITY,
) => {
- if (
- extremeValue === cutoffPoint ||
- typeof cutoffPoint !== 'number' ||
- typeof extremeValue !== 'number' ||
- typeof value !== 'number'
- ) {
+ if (extremeValue === cutoffPoint || typeof value !== 'number') {
return maxOpacity;
}
+ const numCutoffPoint =
+ typeof cutoffPoint === 'string' ? parseFloat(cutoffPoint) : cutoffPoint;
+ const numExtremeValue =
+ typeof extremeValue === 'string' ? parseFloat(extremeValue) : extremeValue;
+
Review Comment:
parseFloat can return NaN for invalid string inputs. After parsing, validate that numCutoffPoint and numExtremeValue are valid numbers using isNaN() or Number.isFinite() before using them in calculations. Add a check like: `if (!Number.isFinite(numCutoffPoint) || !Number.isFinite(numExtremeValue)) { return maxOpacity; }`
```suggestion
if (!Number.isFinite(numCutoffPoint) || !Number.isFinite(numExtremeValue)) {
return maxOpacity;
}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: opacity color formating [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36101:
URL: https://github.com/apache/superset/pull/36101#issuecomment-3528139326
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org