profiler icon indicating copy to clipboard operation
profiler copied to clipboard

Add a fullscreen button to the bottom box

Open eqrion opened this issue 4 months ago • 8 comments

Production | Deploy preview

Fixes #5583. I wasn't sure where to get an icon for this, so I just re-used an existing one. I can look for something better if you have a preferred source for them.

Screenshot 2025-09-05 at 2 18 17 PM

eqrion avatar Sep 05 '25 19:09 eqrion

Codecov Report

:x: Patch coverage is 64.70588% with 12 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 85.60%. Comparing base (2ff1b40) to head (dd0b8cd). :warning: Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/components/app/BottomBox.tsx 60.00% 4 Missing :warning:
src/actions/profile-view.ts 0.00% 3 Missing :warning:
src/components/app/FullscreenToggleButton.tsx 77.77% 2 Missing :warning:
src/reducers/url-state.ts 66.66% 2 Missing :warning:
src/app-logic/url-handling.ts 75.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5605      +/-   ##
==========================================
- Coverage   85.63%   85.60%   -0.03%     
==========================================
  Files         312      313       +1     
  Lines       30892    30927      +35     
  Branches     8420     8519      +99     
==========================================
+ Hits        26453    26475      +22     
- Misses       4009     4022      +13     
  Partials      430      430              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Sep 05 '25 19:09 codecov[bot]

Looks pretty nice! But I think we should add more ways to exit this view.

I can think of these three ways that users might try to get back out of a view that takes over the entire page:

  • Navigate back
  • Close the tab
  • Press Esc

I actually remember myself closing profiler tabs unintentionally in the old "cleopatra" Firefox profile viewer which actually had this type of fullscreen source view, because the context cues somehow made me think I was in a new tab.

So I think we should at least make the "navigate back" interaction work, maybe also the "Press Esc" interaction. For navigating back to un-maximize the source view, the source view maximize state just needs to be stored in the URL state.

It would be cool to have this button open the source code in a new window, so that you can close the tab and not lose your profile. But that's probably a bit complicated. I wonder if we can open an about:blank window, put some minimal DOM in there with document.open/write/close and render into it with ReactDOM. Here's an article I found which describes doing something similar in Electron: https://pietrasiak.com/creating-multi-window-electron-apps-using-react-portals

Anyway, the only requirement I have for now is to make it so that navigating back un-maximizes.

mstange avatar Sep 09 '25 03:09 mstange

So I think we should at least make the "navigate back" interaction work, maybe also the "Press Esc" interaction. For navigating back to un-maximize the source view, the source view maximize state just needs to be stored in the URL state

I will go for this. The portal trick and separate window seems neat, but I'm a bit concerned it's above my React skill level.

Not localization, but if the icon has an arrow going out, it should change for the opposite action (restore). With that said, I think that icon is used for "this link opens in a new window" (even with Profiler), so that's confusing

We should probably use a different icon. I just picked an existing one to have something while testing this out. @mstange or @flodolo do you have a recommendation on where to get a new icon for this?

eqrion avatar Sep 10 '25 16:09 eqrion

I've updated the patch with new messages, new SVG icons for minimize/maximise (from font awesome, license here I think), escape closes fullscreen, and using the history back button closes fullscreen.

eqrion avatar Sep 24 '25 20:09 eqrion

Light review ping @mstange

eqrion avatar Sep 30 '25 15:09 eqrion

I tried out both of the icons, I'm not sure which I prefer. I also moved the icon to be next to close button, as I think it logically goes there.

Here's the photon one: photon

And here's the acorn one: acorn

The PR here has the acorn one now. Let me know which you prefer and I can rollback to the photon one easily.

I also changed it so that closing the bottom box will remove the fullscreen state. I did this by dispatching two actions in this case because I wasn't sure how to have a single reducer update the state for both in the current redux architecture. Let me know if there's a better way to do it.

eqrion avatar Nov 14 '25 16:11 eqrion

Friendly review ping @canova

eqrion avatar Dec 10 '25 17:12 eqrion

@flodolo I believe the changes you requested have been applied, could you approve?

mstange avatar Dec 11 '25 15:12 mstange