Issue #1818 - When searching in the stack chart, the matched nodes should be highlighted #2957
Conversation
Codecov ReportBase: 88.56% // Head: 88.56% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2957 +/- ##
=======================================
Coverage 88.56% 88.56%
=======================================
Files 282 282
Lines 25333 25342 +9
Branches 6817 6821 +4
=======================================
+ Hits 22435 22443 +8
- Misses 2696 2697 +1
Partials 202 202
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. |
There was a problem hiding this comment.
Thanks for the PR @js636f! I'm not sure about the current solution.
Black text on top of a dark blue rectangle doesn't look very accessible. Even I (as a person who has a fairly okay eyesight) have trouble reading the text of the highlighted node :) We can maybe find another way to highlight the matched node.
Also we don't use save and restore in the codebase yet, so it would be good to look t the performance impact of both these values and testing each frame with regexes.
@julienw @gregtatum what do you think about this highlight style?
Example profile
Definitely not accessible Can we please have a look at the solution with a border please? :-) |
|
Sorry it took so long to answer you. TBH I like it this way :-) even for tiny nodes. I think there may be a bug, see this example: I'm looking for "MessageLoop", and it looks like that only half of the matches have their border. But otherwise I think we can move forward this way. What do you think @canova? |
src/components/stack-chart/Canvas.js
Outdated
| @@ -345,6 +348,13 @@ class StackChartCanvasImpl extends React.PureComponent<Props> { | |||
| intW + BORDER_OPACITY, | |||
| intH | |||
| ); | |||
|
|
|||
| if (searchStringsRegExp && searchStringsRegExp.test(text)) { | |||
There was a problem hiding this comment.
Because the searchStringsRegExp is shared, it needs to be "reset" when using it. I think this is the source of the bug I found. You can do this:
profiler/src/components/shared/TreeView.js
Lines 77 to 78 in 510a546
src/components/stack-chart/Canvas.js
Outdated
| @@ -345,6 +348,13 @@ class StackChartCanvasImpl extends React.PureComponent<Props> { | |||
| intW + BORDER_OPACITY, | |||
| intH | |||
| ); | |||
|
|
|||
| if (searchStringsRegExp && searchStringsRegExp.test(text)) { | |||
| ctx.strokeStyle = 'green'; | |||
There was a problem hiding this comment.
I was thinking that the color could be somewhat softer. We can try the mozilla blue BLUE_60 for example. You can import it like this:
import { BLUE_60 } from 'photon-colors'and use it directly.
|
Hey @js636f, is that still something you'd like to move forward to the finish line? That's totally OK if you don't, but then please tell us :-) Thanks for your feedback. |
|
Hey, @julienw ! I'm a little busy, but anyway I hope I'll be able to finish this task. But if it's urgent and someone would like to continue work on it and use my code as a base - It's OK for me. |
|
gently ping @js636f :) |
|
Hello, @julienw I'm still hope to finish the task. If I will not be able to finish it in a week from now please give it to someone else. Thanks! |
|
I still think it would be a valuable addition. But we need to highlight the full call node paths and not just the call nodes. So this needs some more work to be able to do that. I'm keeping the PR open for now. |
The initial proposal was to use a border to highlight the matched nodes in the stack chart. But as it turned out the nodes are too tiny and I couldn't find suitable border size. So I just draw another semitransparent rectangle at the top of the matched node. I don't think this is a production-ready solution (I didn't check performance, the appearance of the highlighting can and should be improved), but now we may discuss it and I will improve it.
Fixes #1818
┆Issue is synchronized with this Jira Task