-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix(widgets): use assigned viewport to change viewState #8903
Conversation
Signed-off-by: Chris Gervang <chris@gervang.com>
@@ -117,7 +117,7 @@ export class CompassWidget implements Widget<CompassWidgetProps> { | |||
} | |||
|
|||
handleCompassReset() { | |||
const viewId = this.viewId || 'default-view'; | |||
const viewId = this.viewId || this.viewport?.id || 'default-view'; |
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.
Seems a bit clunky that this needs to be duplicated across widgets, it would be nice if we could push this complexity into the WidgetManager
somehow. Also, can the this.viewport?.id
ever be different from the viewId
? If not, we could just do:
const viewId = this.viewport!.id || 'default-view';
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.
Widgets implement how they handle this themselves since not every widget is tied to a view (FullscreenWidget has no view code in it).
I think viewId and viewport can differ in multi-view. I have some questions about multi-view widgets to look into next, so I'll follow up on this soon. The intended design is:
- If a
viewId
prop is defined by a user, the widget is placed in that view (i.e. a mini-map) and changes that view. - Otherwise, the widget should be placed in the default top-left view container and change all views.
Closes #8902
Change List