Added cross-browser fullscreen behaviour #47
Conversation
DiLazarova
added
the
✅ status: verified
mddragnev
left a comment
mddragnev
left a comment
There was a problem hiding this comment.
Two more things:
1.
Entering full screen mode there is an additional scrollbar.
- All the checks whether window/document is defined are redundant here because we are using client side code in the entire file. Event if we migrate this project to SSR we should either refactor the whole file or mark it as
'use client'. However, this is something that I can live with so just FYI
| downloadLink: string; | ||
| } | ||
|
|
||
| interface TabItemProps { |
There was a problem hiding this comment.
I think having these as interfaces is much more readable than putting them as types of the arguments in the component
| document.exitFullscreen?.() || | ||
| (document as any).webkitExitFullscreen?.(); | ||
|
|
||
| const checkFullscreen = () => |
There was a problem hiding this comment.
Two identical functions checkFullScreen.
| return; | ||
| if (typeof window === "undefined" || typeof document === "undefined") return; | ||
|
|
||
| const checkFullscreen = () => |
| setActiveView(location.pathname.replace("/home/", "")); | ||
| }, [location]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
What I dont like in this useEffect is that it is too complicated. Do you really need to reattach the event listeners for fullscreen handling on each location change? Isn't it better to split up the Effect into two separate so that the event listeners are added only once, and then the tabs logic is changed on location? Also I dont think that we need all this state. There might be some state that can be derived but this is probably not the time for this.
|
|
||
| const onToggleFullscreen = async () => { | ||
| if (typeof document === "undefined") return; | ||
| const onToggleFullscreen = useCallback(async () => { |
There was a problem hiding this comment.
Why do you need useCallback here?
| await exitFullscreen(); | ||
| } | ||
|
|
||
| setIsFullscreen(checkFullscreen()); |
There was a problem hiding this comment.
Do you need to set it here as well?
You have some event listeners in the effect that should take care of changing the state
| setIsChartsSection(false); | ||
| } | ||
|
|
||
| setActiveView(path); |
There was a problem hiding this comment.
There is a usEffect above that does the same thing? Why dont you remove the one above?
Edit: As I am looking at this, couldnt activeView be a derived variable instead of state?
Related to: IgniteUI/angular-grid-examples#147