Skip to content
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

Create a fullscreen toggle element #440

Merged
merged 8 commits into from
Oct 13, 2023
Merged

Create a fullscreen toggle element #440

merged 8 commits into from
Oct 13, 2023

Conversation

garrettmflynn
Copy link
Member

fix #428

@garrettmflynn garrettmflynn self-assigned this Oct 11, 2023
@CodyCBakerPhD
Copy link
Collaborator

This really looks amazing when the screen is maximized - dare I say even better than the main neurosift site?

Only minor thing here, screenshot below

Screenshot 2023-10-13 at 4 49 00 AM

Not clear that the fullscreen button is for the nested neurosift window; any way to pin it to the upper-right of the nested window?

@CodyCBakerPhD
Copy link
Collaborator

Similarly, for a single-session preview, it looks like this for me by default

Screenshot 2023-10-13 at 4 51 56 AM

'Inspector Report' gets clipped off even when in fullscreen, and the fullscreen button partially occludes the 'Inspector Report' title, even in fullscreen

@CodyCBakerPhD
Copy link
Collaborator

Other than that looks great!

@garrettmflynn
Copy link
Member Author

Fixed the former—but for the latter, I'd assumed you wanted the Inspector Report to be included in the maximization. Hence why it's pinned to the limits of both rather than just Neurosift.

When there are actual warnings populating the list, there isn't any meaningful information occluded:
Screenshot 2023-10-13 at 9 00 03 AM

Furthermore, if the structure of Neurosift itself changes (e.g. no navigation bar), this will become an issue as well.

As a tentative fix, I've made the toggle semi-transparent, transitioning the opacity to 100% when the user hovers over it.
Screenshot 2023-10-13 at 9 08 59 AM

Does this work as a more general fix for the latter case?

@garrettmflynn
Copy link
Member Author

I've also added a minWidth + emptyMessage content to the Inspector List in this case so that the header is never occluded.
Screenshot 2023-10-13 at 9 17 38 AM

@CodyCBakerPhD
Copy link
Collaborator

I'd assumed you wanted the Inspector Report to be included in the maximization. Hence why it's pinned to the limits of both rather than just Neurosift.

Absolutely, the full screen is especially better at reporting both

My minor annoyance was with the text clipping and button overlay not looking great, but

As a tentative fix, I've made the toggle semi-transparent, transitioning the opacity to 100% when the user hovers over it.
I've also added a minWidth + emptyMessage content to the Inspector List in this case so that the header is never occluded.

Both do a great job of alleviating that issue!

@CodyCBakerPhD CodyCBakerPhD merged commit efad883 into main Oct 13, 2023
7 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the fullscreen branch October 13, 2023 18:18
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.

[Feature] Window expansion of Neurosift and single-session preview page
2 participants