Skip to content

Only count markers that are part of the selected range when hovering a marker in the marker chart.#5004

Merged
fqueze merged 1 commit into
firefox-devtools:mainfrom
fqueze:marker-count-for-preview
May 22, 2024
Merged

Only count markers that are part of the selected range when hovering a marker in the marker chart.#5004
fqueze merged 1 commit into
firefox-devtools:mainfrom
fqueze:marker-count-for-preview

Conversation

@fqueze

@fqueze fqueze commented May 21, 2024

Copy link
Copy Markdown
Contributor

Fixes a bug in the implementation of #4892.

Steps to reproduce:

  • Load this example profile: https://share.firefox.dev/3wMQqtN
  • Hover a marker and see the marker count
  • Change the preview selection
  • See that less markers are drawn
  • Hover a marker. Notice the marker count is still the same as before; it should be lower.

@fqueze fqueze requested a review from julienw May 21, 2024 08:53
@codecov

codecov Bot commented May 21, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.46%. Comparing base (ffe2b6a) to head (1a18805).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5004   +/-   ##
=======================================
  Coverage   88.46%   88.46%           
=======================================
  Files         304      304           
  Lines       27317    27329   +12     
  Branches     7377     7378    +1     
=======================================
+ Hits        24165    24177   +12     
  Misses       2930     2930           
  Partials      222      222           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@julienw julienw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super happy about this duplication, but let's do that...
I wanted to ask you about improving the tests, but I see there's no existing test for the marker chart when a preview selection is active 😱. If you want to add one I'd be glad but I won't make you do one.

@fqueze

fqueze commented May 22, 2024

Copy link
Copy Markdown
Contributor Author

Thanks for the review!

I'm not super happy about this duplication, but let's do that...

I'm not super happy about the duplication either. I initially wanted to make a shared method returning timeAtViewportLeft and timeAtViewportRightPlusMargin, but then I realized some of the intermediary variables are used for the other caller, so I wouldn't actually win much.

@fqueze fqueze enabled auto-merge (squash) May 22, 2024 15:46
@fqueze fqueze force-pushed the marker-count-for-preview branch from 1a18805 to b61dd9f Compare May 22, 2024 15:47
@fqueze fqueze merged commit 07b78f6 into firefox-devtools:main May 22, 2024
@julienw julienw mentioned this pull request Jun 4, 2024
julienw added a commit that referenced this pull request Jun 4, 2024
[Florian Quèze] Only count markers that are part of the selected range when hovering a marker in the marker chart. (#5004)
[Valentin Gosu] Add sanitized-string format (#5007)

Thanks also to our localizers:
zh-CN: Olvcpr423
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants