Display the vertical line in the timeline when hovering the marker chart or stack chart, fixes #222.#4742
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4742 +/- ##
==========================================
+ Coverage 88.32% 88.34% +0.01%
==========================================
Files 300 300
Lines 26734 26767 +33
Branches 7214 7219 +5
==========================================
+ Hits 23612 23646 +34
Misses 2909 2909
+ Partials 213 212 -1
☔ View full report in Codecov by Sentry. |
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
I believe we should pass the React events to these handlers, but otherwise this looks pretty good to me.
In terms of testing, you could add something in src/test/components/MarkerChart.test.js , you can see how we use fireMouseEvent in some tests. For example you could fire a mousemove event, and then check the value of the mouse position in the store, fire another mousemove event and then check this moved, and finally fire a mouseleave event and check the value is null.
Tell me what you think!
| +onMouseMove?: MouseEventHandler, | ||
| +onMouseLeave?: MouseEventHandler, |
There was a problem hiding this comment.
Currently, your design makes these functions take a DOM MouseEvent (and you pass it nativeEvent). I think it would be less surprising if we passed the React event instead. To access offsetX I believe we still need to use nativeEvent (at least according to Flow) so that shouldn't change your code otherwise.
…art or stack chart, fixes firefox-devtools#222.
fqueze
force-pushed
the
vertical-line
branch
from
64356c3 to
5a691d8
Compare
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
lgtm, thanks for the test!
* main: (37 commits) Show the hovered time in the ruler at the top of the timeline. (PR firefox-devtools#4748) Hardcode 'project' to 'firefox-profiler' in .taskcluster.yml (PR firefox-devtools#4759) Update all Yarn dependencies (2023-09-27) (PR firefox-devtools#4758) Update the uploading command in the developer documentation (PR firefox-devtools#4752) Make prettier ignore the taskcluster files Add taskcluster Update all Yarn dependencies (2023-09-20) (PR firefox-devtools#4756) Add a robots.txt file to disallow indexing our profile links (PR firefox-devtools#4753) Avoid showing the calltree panel in profiles without samples. (PR firefox-devtools#4744) Display the vertical line in the timeline when hovering the marker chart or stack chart (PR firefox-devtools#4742) Draw a single rectangle for 2 markers in bar charts only if both the start and end horizontal pixels match. (PR firefox-devtools#4734) Update all Yarn dependencies (2023-09-13) (firefox-devtools#4746) Update all Yarn dependencies (2023-09-06) (PR firefox-devtools#4741) Remove prefer-wait-for rule in our config because it's been removed from the plugin ⬆️ Update eslint-plugin-testing-library to version 6.0.1 Use a cache for prettier (PR firefox-devtools#4731) Remove leftover eslint error Run prettier on the codebase Upgrade prettier to latest version Ignore the coverage directory when present ...
2 participants
Fixes #222
This PR does the PR 3 and PR 4 work mentioned in Greg's comment explaining what were the things to do.
I haven't included tests, if we think we need some, I would need a bit of help to figure out where they should be, and what they should do.
The code in the new onMouseMove methods has some duplication with the hitTest methods. It could be possible to factor out some of it (but a new method would need to return both the xInUnitInterval and xInTime values), not sure it's worth it.
This only displays the vertical line in the timeline when hovering the marker chart of stack chart. Ideally we would want to show vertical lines in these two panels too, but I thought this small PR was already useful enough to be worth pushing, and we could do the rest in future PRs.