fix: color collision in dashboard with tabs #24670
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lilykuang
requested review from
jinghua-qa,
geido,
eschutho,
rusackas and
betodealmeida
as code owners
Codecov Report
@@ Coverage Diff @@
## master #24670 +/- ##
=======================================
Coverage 68.97% 68.97%
=======================================
Files 1901 1901
Lines 74006 74008 +2
Branches 8182 8183 +1
=======================================
+ Hits 51045 51047 +2
Misses 20840 20840
Partials 2121 2121
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
eschutho
reviewed
Jul 12, 2023
| Object.entries(this.parentForcedColors).forEach(([key, value]) => { | ||
| if (key !== cleanedValue) { | ||
| const index = updatedRange.indexOf(value); | ||
| updatedRange.splice(index, 1); |
There was a problem hiding this comment.
could you use a filter method on updatedRange here instead? That may be a little more succinct.
There was a problem hiding this comment.
Additional suggestion: #24670 (comment)
eschutho
approved these changes
Jul 12, 2023
lilykuang
force-pushed
the
fix-color-collision-tabs
branch
2 times, most recently
from
339b33a
to
e62cee9
Compare
justinpark
reviewed
Jul 13, 2023
Comment on lines
81
to
92
| 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 |
There was a problem hiding this comment.
The combination of forEach and .filter operation can be heavy. It should be better using a hashset to filter out the values.
Suggested change
| 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 | |
| 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); |
There was a problem hiding this comment.
oh nice. i will use that
lilykuang
force-pushed
the
fix-color-collision-tabs
branch
2 times, most recently
from
6d42537
to
5df4705
Compare
lilykuang
force-pushed
the
fix-color-collision-tabs
branch
from
c7a50b4
to
622ae33
Compare
michael-s-molina
added
the
v3.0
michael-s-molina
pushed a commit
that referenced
this pull request
Jul 26, 2023
(cherry picked from commit 0328dd2)
mistercrunch
added
🍒 3.0.0
🍒 3.0.1
🍒 3.0.2
🍒 3.0.3
🍒 3.0.4
🏷️ bot
vinothkumar66
pushed a commit
to vinothkumar66/superset
that referenced
this pull request
Nov 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION