-
Notifications
You must be signed in to change notification settings - Fork 787
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
Create Share and GetConnected apps #622
base: master
Are you sure you want to change the base?
Conversation
export class GetConnectedApp { | ||
constructor(private dialog: polymer.Base) { | ||
// Get connected is not a Polymer component, so we use `querySelector()` instead of `dialog.$`. | ||
dialog.querySelector('#closeGetConnectedButton') |
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.
getConnectedDialog
is a paper-dialog
and you're passing a polymer.Base
object, so it's not clear to me why dialog
is not a Polymer component.
@@ -145,6 +147,9 @@ export class App { | |||
private digitalOceanTokenManager: TokenManager, private surveys: Surveys) { | |||
appRoot.setAttribute('outline-version', this.version); | |||
|
|||
const shareApp = new ShareDialogApp(appRoot.$.shareDialog); | |||
const getConnectedApp = new GetConnectedApp(appRoot.$.getConnectedDialog); |
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.
nit: s/App/Controller
. I think app is not as intuitive to understand the relationship of the class with the view element.
This PR makes progress and is an example of:
The impact of this change is not huge, but we will see a much bigger impact if we use a similar pattern in larger components, like the server view.
cc: @JonathanDCohen @mpmcroy