Fix some category colors and add magenta#4347
Conversation
Codecov ReportBase: 88.38% // Head: 88.37% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4347 +/- ##
==========================================
- Coverage 88.38% 88.37% -0.02%
==========================================
Files 282 282
Lines 25310 25314 +4
Branches 6823 6826 +3
==========================================
+ Hits 22370 22371 +1
- Misses 2728 2731 +3
Partials 212 212
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| --category-color-brown: var(--orange-70); | ||
| --category-color-magenta: var(--magenta-60); | ||
|
|
||
| /* it is hard to create a lighter version of the following colors */ |
There was a problem hiding this comment.
this distinction was also implemented as part of #4193, for an initial version of the patch, but in the end it wasn't used. What do you think about removing the comment and merging them in the same list?
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for working on it.
It will take me some time to get used to the new shade of gray color of "other" category 😄
| selectedTextColor: '#fff', | ||
| gravity: 7, | ||
| }; | ||
| case 'magenta': |
| selectedFillStyle: MAGENTA_60, | ||
| unselectedFillStyle: MAGENTA_60 + '60', | ||
| selectedTextColor: '#fff', | ||
| gravity: 7, |
There was a problem hiding this comment.
Should we keep 7 for the gravity here? I think we can also put it to the further back, but don't have a strong opinion on it.
There was a problem hiding this comment.
So, I thought about this, and realized I have no idea :D I believe some of the existing gravities are quite bad but didn't want to change them all. That's where my thinking about moving away the gravity value into the category information in the profile metadata came from too... it seems a bad design decision that the gravity is tied to colors rather than categories... So I decided to... leave it.
I'm happy to put a bigger value if you think it's better!
There was a problem hiding this comment.
I used 8 and increased all the ones below.
| --category-color-brown: var(--orange-70); | ||
| --category-color-magenta: var(--magenta-60); | ||
|
|
||
| /* it is hard to create a lighter version of the following colors */ |
Let's pay attention to feedback too; in case it's too dark we can always adjust more in the future. |
2 participants
This makes the following changes (each in its own commit):
lightredreally light red in the CSS tooltips and the call tree.greyanddarkgrayand I didn't like this inconsistency much)The order for these commits isn't the most logical, I think the reorder should have come earlier, but reording the commit would yield some conflicts and I admit I didn't want to resolve them. I think it's still OK.
Tell me what you think!
Production
deploy preview
As a reminder, the CSS colors are used in the tooltips and the call tree, while the colors from colors.js are used in the activity graph as well as the flame graph and the stack chart.