-
Notifications
You must be signed in to change notification settings - Fork 210
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
area/ui: Add legend to icicle graph when comparing profiles #993
Conversation
f13fb30
to
d35a36a
Compare
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.
Niiiice! lgtm 👍 🎉
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.
Looks good except the minor feedback.
const newSpanColor = isDarkMode ? '#B3BAE1' : '#929FEB'; | ||
const getIncreasedSpanColor = (transparency: number) => { | ||
return isDarkMode | ||
? `rgba(255, 177, 204, ${transparency})` |
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.
Is it possible to share these color contants/logic from the shared/functions
package file above?
}) => { | ||
const isDarkMode = useAppSelector(selectDarkMode); | ||
|
||
const newSpanColor = isDarkMode ? '#B3BAE1' : '#929FEB'; |
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.
Agreed with Manoj's comment below - maybe we can save these hex codes or rgb values as a constant somewhere that communicates their meaning/purpose?
<div className="flex items-center justify-center"></div> | ||
<span className="block text-sm text-gray-500 dark:text-gray-50"> | ||
This is a differential icicle graph, where the purple means unchanged, and the | ||
darker the red, the worse it got, and the darker the green, the better it got. |
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.
When we say "it", will the user know what "it" was that got worse or better?
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.
I reworded to this: This is a differential icicle graph, where a purple-colored node means unchanged, and the darker the red, the worse the node got, and the darker the green, the better the node got.
Looks amazing! 😎 As for the wording I'm wondering if "better" and "worse" might be better suited to describe the difference since it's all relative? |
pls have a look at this again when you can @manojVivek @monicawoj |
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.
lgtm!
This PR adds a legend for the icicle graph when comparing two profiles so users understand what the colors mean. Additional textual information is shown when you hover on the legend.
Resolves #463