-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Scroll the document when scrollTop message is sent from viewer #6679
Conversation
966919d
to
8bfd935
Compare
5ab1d96
to
57d94cb
Compare
@@ -130,6 +130,9 @@ export class Viewer { | |||
/** @private {!Observable<!JSONType>} */ | |||
this.broadcastObservable_ = new Observable(); | |||
|
|||
/** @private {!Observable<!JSONType>} */ | |||
this.scrollDocObservable_ = new Observable(); |
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 it make sense to introduce more API dependencies when we are in the process of moving them out? Let's just create a generic observable for all types of messages?
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.
What do you mean? Is there a PR where we are moving out the API dependencies? Do you mean changing this to the following?
/** @private {!Observable} */
this.scrollDocObservable_ = new Observable();
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.
There'd be a map of observables: !Object<string, !Observable<!JSONType>>
. The index is the type of message. The API will be viewer.onMessage(eventType, handler)
. The records in the map will be created from calls to this API. Thus we simply remove a switch if (eventType == 'x') {x.fire(payload);}
to simply if (observables[eventType]) {observables[eventType].fire(payload)}
.
57d94cb
to
a6078a4
Compare
a7bfa7a
to
b164d49
Compare
b164d49
to
b9298b8
Compare
Rebased after refactoring observables in |
Fix #6588