-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
48bee57
to
3d69f31
Compare
Quality Gate passedIssues Measures |
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.
@juancarlosfarah Thank you for the PR, here are my first impressions:
From my understanding of the current implementation:
- the "fake" fullscreen can only be toggled with the query param
fullscreen=true
- once inside the fake fullscreen you see the "real" fullscreen button (it says "enter fullscreen")
- There is nowhere in the interface to remove the "fake" fullscreen. I fear users might get stuck in there. Also how can you access it if you do not know that there is a special query you can add ? Is it intended to be "secret" ?
I have an issue with the cookie banner covering the navigation, so if I never accept nor deny the cookies I will never see the navigation ? Also the bottom bar can cover the end of the page on scroll. We should add a safety padding at the end of the page.
We already have a fullscreen hook (it should work (used in the code capsule app), as it is also based on the Fullscreen API https://github.com/graasp/graasp-ui/blob/d822fdeb7e0c8db7c836ff86f283ba5dfc5da65c/src/hooks/useFullscreen.tsx#L8)
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 good ! Thank you for the changes ! 🏆
@spaenleh thanks for your review. Will go ahead and merge. For the cookie banner issue, we can tackle it in another branch. How do you want to do that? Just padding? |
@juancarlosfarah Not sure, maybe we just leave it as is, this issue is also present in the builder for example, it obstructs some part of the UI. |
Exactly. I mentioned that in my comment which was sent concurrently with yours. |
@pyphilia @spaenleh, here's the implementation of the full screen. It includes "fake" and "real" full screen mode.
Fake
To activate, you need to append
?fullscreen=true
to the URL. The left side bar and the header will disappear. Like this, it's a "fake" full screen. You can navigate with the bar in the bottom.Real
Only within "fake" full screen mode (for now), you can activate the real full screen mode by clicking on an icon button.
I'm also including a fix where the navigation footer hides the cookie banner.
Do you think we can send this to staging soon? Thanks!