-
Notifications
You must be signed in to change notification settings - Fork 450
Allow markers to provide a marker color. #5607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5607 +/- ##
==========================================
+ Coverage 85.80% 85.82% +0.02%
==========================================
Files 309 309
Lines 30381 30407 +26
Branches 8365 8370 +5
==========================================
+ Hits 26068 26097 +29
+ Misses 3892 3889 -3
Partials 421 421 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
20fcc62 to
4df4352
Compare
mstange
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems useful and the implementation looks fine.
The color contrast for hovered markers with the white text isn't great, but this can be improved in a follow-up.
I'm a bit concerned that we now have two sets of "accepted colors" along with "normal and highlighted" versions of each color. How bad would it be to reuse the category colors for these marker colors? The category colors have proven themselves to some degree in the stack chart and flame graph, for example I think they mostly have reasonable color contrast with the text that's on them, or at least if there were cases of really bad contrast we would probably have done something about it by now.
Thanks!
Done in #5609.
We already had these 2 sets of accepted colors when we merged support for marker tracks. That second set of colors solved two problems:
Additionally, color.ts color consts for the teal and ink colors, but no associated color styles. I would also prefer if we could merge these 2 sets of colors to have only one, but it would take significant effort to address the various edge cases. |
Changes: [Markus Stange] Switch CSS back to using relative paths inside url(...) (#5594) [Markus Stange] Use dynamic imports for jszip (#5593) [Florian Quèze] Allow markers to provide a marker color. (#5607) [Ryan Hunt] Update iongraph-web to latest version (#5606) [Florian Quèze] Fix the color contrast of hovered colored markers. (#5609) [Florian Quèze] Scale custom marker graphs on values within the committed range. (#5587) [Markus Stange] Fix null marker stack substitution (#5613) [Florian Quèze] Track context menus should be accessible from anywhere in the track (#5562) [Markus Stange] Worker + compression cleanups (#5602)

Adds an optional
colorFieldfield to marker schemas that specifies which marker data field contains aGraphColorvalue, allowing individual markers to render with different colors based on their data.deploy preview