-
Notifications
You must be signed in to change notification settings - Fork 7
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
Allow custom html in title #51
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.
Nice! We are almost there! 😃
I have some comments bellow and sorry for not being able to review it right away.
Reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @tulioag and @DiegoCardoso)
demo/element-basic-demos.html, line 59 at r1 (raw file):
<vaadin-demo-snippet id="login-overlay"> <template preserve-content> <vaadin-login-overlay><h1 slot="title">My App</h1></vaadin-login-overlay>
nit: indentation is not correct.
Also, I am wondering whether it makes sense to show this feature here or in another example. It may make more sense to have it in an advanced tab, so we can keep the basic examples simple. What do you think?
src/vaadin-login-overlay.html, line 110 at r1 (raw file):
} _createTeleporter() {
Why aren't teleport
and unteleport
functions declared straight as part of VaadinLoginOverlay
? teleported
could be a private member of the class, as it makes sense to keep it cached. That way, we wouldn't need to create this for every instance of vaadin-login-overlay
.
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.
Reviewable status: 1 of 8 files reviewed, 2 unresolved discussions (waiting on @DiegoCardoso)
demo/element-basic-demos.html, line 59 at r1 (raw file):
Previously, DiegoCardoso (Diego Cardoso) wrote…
nit: indentation is not correct.
Also, I am wondering whether it makes sense to show this feature here or in another example. It may make more sense to have it in an advanced tab, so we can keep the basic examples simple. What do you think?
Agreed. Done.
src/vaadin-login-overlay.html, line 110 at r1 (raw file):
Previously, DiegoCardoso (Diego Cardoso) wrote…
Why aren't
teleport
andunteleport
functions declared straight as part ofVaadinLoginOverlay
?teleported
could be a private member of the class, as it makes sense to keep it cached. That way, we wouldn't need to create this for every instance ofvaadin-login-overlay
.
The idea was to group the functionality in one opaque object. I refactored this so now the _teleport
function just returns a function to undo the teleport. Besides that, I added caching for the query .
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.
Reviewable status: 1 of 8 files reviewed, 1 unresolved discussion (waiting on @DiegoCardoso)
src/vaadin-login-overlay.html, line 110 at r1 (raw file):
Previously, tulioag (Tulio Garcia) wrote…
The idea was to group the functionality in one opaque object. I refactored this so now the
_teleport
function just returns a function to undo the teleport. Besides that, I added caching for the query .
Done.
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.
Reviewed 2 of 3 files at r2, 4 of 5 files at r3.
Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @alexberazouski, @DiegoCardoso, and @tulioag)
demo/advanced-demos.html, line 12 at r3 (raw file):
<vaadin-demo-snippet id="login-overlay"> <template preserve-content> <vaadin-login-overlay><h1 slot="title"><iron-icon icon="vaadin:vaadin-h"></iron-icon> My App</h1></vaadin-login-overlay>
nitpick: Icon and text are misaligned
Closes #10
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)