-
Notifications
You must be signed in to change notification settings - Fork 116
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(controls): Add Settings menu to Model3D #1377
Conversation
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.
LGTM overall, nice job.
908621c
to
5e31698
Compare
}, []); | ||
|
||
onClose(); | ||
}, [onClose]); |
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.
It seems like any call to Model3dViewer's renderUI
would reset the controls, since the handler is declared as an inline function. Is that not the case? Would it be safer to create a useCallback
helper that can both call setIsOpen
and onClose
or onOpen
?
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.
No, that's not the case. Seems like any call to renderUI
should probably create a new callback for resetControls
but not necessarily call it
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.
Are you proposing something like:
const setIsOpenHelper = React.useCallback(() => {
const nextIsOpen = !isOpen;
setIsOpen(nextIsOpen);
if (nextIsOpen) {
onOpen();
} else {
onClose();
}
}, [onClose, onOpen]);
And use that in place of setIsOpen
?
@@ -11,7 +11,7 @@ $bp-SettingsDropdown-spacing: 5px; | |||
} | |||
|
|||
.bp-SettingsDropdown-flyout { | |||
top: initial; | |||
top: auto; |
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.
Does this affect the media controls at all?
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.
No this is an override only for SettingsDropdown
TODO:
Before:
After: