[GitHub] [superset] justinpark commented on a diff in pull request #24670: fix: color collision in dashboard with tabs

["justinpark (via GitHub)" <gi...@apache.org>]

justinpark commented on code in PR #24670:
URL: https://github.com/apache/superset/pull/24670#discussion_r1262907425


##########
superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts:
##########
@@ -78,14 +78,18 @@ class CategoricalColorScale extends ExtensibleFunction {
     cleanedValue: string,
   ) {
     // make sure we don't overwrite the origin colors
-    const updatedRange = [...this.originColors];
+    let updatedRange = [...this.originColors];
     // remove the color option from shared color
     sharedColorMap.forEach((value: string, key: string) => {
       if (key !== cleanedValue) {
-        const index = updatedRange.indexOf(value);
-        updatedRange.splice(index, 1);
+        updatedRange = updatedRange.filter(color => color !== value);
       }
     });
+    Object.entries(this.parentForcedColors).forEach(([key, value]) => {
+      if (key !== cleanedValue) {
+        updatedRange = updatedRange.filter(color => color !== value);
+      }
+    }); // remove the color option from forced colors

Review Comment:
   The combination of `forEach` and `.filter` operation can be heavy. It should be better using a hashset to filter out the values.
   
   ```suggestion
       const updatedRange = new Set(this.originColors);
       // remove the color option from shared color
       sharedColorMap.forEach((value: string, key: string) => {
         if (key !== cleanedValue) {
           updatedRange.delete(value);
         }
       }
       Object.entries(this.parentForcedColors).forEach(([key, value]) => {
         if (key !== cleanedValue) {
           updatedRange.delete(value);
         }
       }
       this.range(updatedRange.size > 0 ? [...updatedRange] : this.originColors);
   ```



-- 
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