feat(md): New option to display environments side by side #9269
Conversation
ranihorev
changed the title
ranihorev
force-pushed
the
md/side-by-side
branch
4 times, most recently
from
0187438 to
2762c60
Compare
| React.useLayoutEffect(() => { | ||
| const debouncedResizeHandler = debounce(() => { | ||
| setDimension({ width: window.innerWidth, height: window.innerHeight }); | ||
| }, delay); | ||
| if (isActive) { | ||
| window.addEventListener('resize', debouncedResizeHandler); | ||
| return () => window.removeEventListener('resize', debouncedResizeHandler); | ||
| } else { | ||
| return () => {}; | ||
| } | ||
| }, [delay, isActive]); | ||
|
|
||
| React.useLayoutEffect(() => { | ||
| if (isActive) { | ||
| setDimension({ width: window.innerWidth, height: window.innerHeight }); | ||
| } | ||
| }, [isActive]); |
There was a problem hiding this comment.
It would be helpful to add comments describing what is going on here. Why are there two uses of useLayoutEffect and what does each one of them do? Also describe the purpose of delay and isActive. I can't quite figure out what they do within the context of this file.
There was a problem hiding this comment.
Just confirmed offline that this is NOT used at all. So going to ignore it and look at the rest. Please remove this part before merging the PR.
| }, delay); | ||
|
|
||
| if (isActive && ref.current) { | ||
| const observer = new ResizeObserver(debouncedResizeHandler); |
ranihorev
force-pushed
the
md/side-by-side
branch
from
2762c60 to
ede4bfe
Compare
| // The goal of this hook is to store the value in an atom to be shared across the app but also update the local storage | ||
| const useEnvironmentDirection = () => { | ||
| const [direction, setDirection] = useRecoilState(environmentsDirectionState); | ||
| React.useLayoutEffect(() => { |
There was a problem hiding this comment.
I can't think of a reason why useLayoutEffect would be appropriate here (as opposed to regular old useEffect). Can you explain?
There was a problem hiding this comment.
You can't think of a reason because there is no reason :)
Probably a left over...
| const getElementDimensions = (ref: React.RefObject<HTMLElement>) => | ||
| ref.current ? { width: ref.current.offsetWidth, height: ref.current.offsetHeight } : { width: 0, height: 0 }; | ||
|
|
||
| export const useElementDimensions = ({ |
There was a problem hiding this comment.
the filename should probably match the name of the hook
| ref.current ? { width: ref.current.offsetWidth, height: ref.current.offsetHeight } : { width: 0, height: 0 }; | ||
|
|
||
| export const useElementDimensions = ({ | ||
| ref, |
There was a problem hiding this comment.
this is just a style preference comment, not feedback per se
When an arg is "required" I prefer to pass it as a parameter, not as part of an object. i.e., I would have probably written this as:
export const useElementDimension = (ref: ..., options: ...) =>
33708ee feat(md): change the deploying state blinking green instead of teal (#9291) 995d622 feat(md): set the new UI as the default one, and allow users to opt out (#9290) 3bb4ffe feat(md): show delivery config yml in the configuration tab (#9286) 75a3a25 fix(md): minor improvements based on feedback from #9269 (#9289)
33708ee feat(md): change the deploying state blinking green instead of teal (#9291) 995d622 feat(md): set the new UI as the default one, and allow users to opt out (#9290) 3bb4ffe feat(md): show delivery config yml in the configuration tab (#9286) 75a3a25 fix(md): minor improvements based on feedback from #9269 (#9289)
To better utilize large screens, I'm trying to display environments side by side. We might want to make it the default but, for now, users can pick their preferred view.