-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat(react-components): Move global camera controls to Reveal context #4745
feat(react-components): Move global camera controls to Reveal context #4745
Conversation
return null; | ||
}; | ||
|
||
export const useReveal = (): Cognite3DViewer => { |
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.
This should be in hooks
folder?
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.
I don't think so, the convention we have been using so far, is that hooks that are sort-of exposing values from a Context, are put together with that Context. That's how this hook was placed before this PR as well, in ViewerContext.ts
*/ | ||
|
||
import { type MutableRefObject, useEffect, useRef } from 'react'; | ||
import { useReveal } from '../../..'; |
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.
Proper path!
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, nice works with the tests!
Minor comments
Type of change
Jira ticket 📘
https://cognitedata.atlassian.net/browse/
Description 📝
This moves the global camera controls to props in RevealContext. Earlier, they would be given to a component that had been created from the
withCameraStateControl
. This makes it very abritrary where in the code this camera control happens, and can be controlled from multiple places in the same page, which may cause unintended behavior. Now, the controls (a getter and setter) of the camera state are given as props directly to RevealContext, making it more obvious where this global state is controlled. Of course, the user may still mutate the state otherwise (either through the useNavigation hook, direct access to the Reveal camera manager, or just moving the camera with mouse or keyboard), and this will be handled gracefully by the RevealContext (the setter sent to RevealContext will be called in all these cases)How has this been tested? 🔍
Test instructions ℹ️
Checklist ☑️