-
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
Extract title/description to vaadin-login-overlay #48
Conversation
Pull Request Test Coverage Report for Build 185
💛 - Coveralls |
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.
Great work, thanks.
I only found a few issues with the demos:
- I18n demo looks ugly. Can we apply the same styles as in the "Basic" version?
- Should we add a demo with the overlay without full screen? I think it looks much better with the overlay and the full screen version is hidden by default.
- Full screen demo does not work in IE11.
Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DiegoCardoso)
test/login-test.html, line 108 at r1 (raw file):
login.i18n = i18n; expect(additionalInformation.textContent).to.be.equal(login.i18n.additionalInformation); // expect(login.title).to.be.equal(login.i18n.additionalInformation);
Can we remove the commented out code?
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 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DiegoCardoso)
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: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @tulioag)
test/login-test.html, line 108 at r1 (raw file):
Previously, tulioag (Tulio Garcia) wrote…
Can we remove the commented out code?
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 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
bd9f76e
to
2af45c3
Compare
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 3 of 3 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @web-padawan)
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: all files reviewed, 4 unresolved discussions (waiting on @web-padawan)
- Make vaadin-login-overlay-element extend vaadin-overlay template - Move title/description to vaadin-login-overlay - Remove props from mixin - Pass through to vloe props set on vaadin-login-overlay - Move styles to vaadin-login-overlary-element - Move tests related to title/description Fix #45
- Fix i18n demo looking - Fix vaadin-login max-width and background - Fix inputs focus ring being cropped
Remove ES6 code from samples
- Incorrect indentation - Remove notify not needed for title/description
2af45c3
to
d4dddd9
Compare
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 4 of 4 files at r5.
Dismissed @web-padawan from 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved
Fix #45
This change is