Skip to content

Show the duration of the full range in the filter navigator bar#3964

Merged
canova merged 1 commit into
firefox-devtools:mainfrom
canova:full-range-duration
Mar 30, 2022
Merged

Show the duration of the full range in the filter navigator bar#3964
canova merged 1 commit into
firefox-devtools:mainfrom
canova:full-range-duration

Conversation

@canova

@canova canova commented Mar 30, 2022

Copy link
Copy Markdown
Member

Fixes #3692

Before:
Screen Shot 2022-03-30 at 3 41 02 PM
After:
Screen Shot 2022-03-30 at 3 40 35 PM

Deploy preview / Production

@canova canova requested a review from a team as a code owner March 30, 2022 13:45
@canova canova requested a review from julienw March 30, 2022 13:46
@codecov

codecov Bot commented Mar 30, 2022

Copy link
Copy Markdown

Codecov Report

Merging #3964 (0da0324) into main (9a16911) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 0da0324 differs from pull request most recent head 17e8ee5. Consider uploading reports for the commit 17e8ee5 to get more accurate results

@@            Coverage Diff             @@
##             main    #3964      +/-   ##
==========================================
- Coverage   87.26%   87.26%   -0.01%     
==========================================
  Files         279      279              
  Lines       23924    23921       -3     
  Branches     6317     6315       -2     
==========================================
- Hits        20877    20874       -3     
  Misses       2807     2807              
  Partials      240      240              
Impacted Files Coverage Δ
src/components/app/ProfileFilterNavigator.js 95.45% <ø> (ø)
src/components/shared/FilterNavigatorBar.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a16911...17e8ee5. Read the comment docs.

class="filterNavigatorBarItemContent"
>
Full Range
Full Range (⁨51ms⁩)

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 curious: what hidden character do you have, and why is it needed?

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.

On second thoughts, I guess they are the directionality characters from Fluent, but didn't realize you need to replicate them in tests.

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 believe these are the isolation characters inserted by Fluent automatically

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.

On second thoughts, I guess they are the directionality characters from Fluent, but didn't realize you need to replicate them in tests.

Well we use Fluent in the tests :-)

@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.

This looks good to me, thanks!

<Localized
id="ProfileFilterNavigator--full-range-with-duration"
vars={{
fullRangeDuration: getFormattedTimeLength(

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.

In the future I'd like to replace this with a Fluent function, but that's a story for later...

>
<>
Full Range (
{getFormattedTimeLength(rootRange.end - rootRange.start)})

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.

What do you think of not using ({getFormattedTimeLength(rootRange.end - rootRange.start)}) here, since it will be replaced by Fluent anyway, it seems counterproductive to run the full process in this case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I can remove this. I really want to bite the bullet at some point and remove the texts from all Localized components. But that's a lot of change...

@canova canova force-pushed the full-range-duration branch from 0da0324 to 17e8ee5 Compare March 30, 2022 14:24
@canova canova merged commit b84f498 into firefox-devtools:main Mar 30, 2022
@canova canova deleted the full-range-duration branch March 30, 2022 14:28
@canova canova mentioned this pull request Apr 5, 2022
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.

We should show the total profile duration next to "Full Range"

3 participants