-
Notifications
You must be signed in to change notification settings - Fork 449
Add a fullscreen button to the bottom box #5605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is 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 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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:
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. |
flodolo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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? |
|
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. |
|
Light review ping @mstange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @eqrion for the PR and sorry for the late review!
I'm looking at the current icon in the deploy preview, and I think it's not looking very uniform with the other icons:

We generally use one from photon icons when possible. I think using icons from there will fit in our UI better.
For example, I see these for the full screen and exit:
https://icons.design.firefox.com/viewer/#fullscreen
Can you try with these icons and see if they indeed fit better? Otherwise we can try the icorn icons here too (inside 16x16):
https://acorn.firefox.com/latest/desktop/styles/iconography-OJ7DtlvA
(acorn is the new design system and the photon is the old one)
I also noticed a subtle issue. STR:
- Open the bottom box and make it full screen.
- Instead of minimizing, close the full screen view directly by clicking the "X".
- Double click another function to open the bottom box.
I would expect the bottom box to open again in non-minimized version. But currently it opens up full screen. I think it makes sense to reset this isBottomBoxFullscreen state when we close the bottom box too. What do you think?
|
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. 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. |
|
Friendly review ping @canova |
mstange
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! Thank you!
|
@flodolo I believe the changes you requested have been applied, could you approve? |


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.