Lightbox(es) should support exiting with escape and x button #1029
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
camelburrito
force-pushed
the
lightbox
branch
from
b43bd6f to
c3bf727
Compare
|
cc @cramforce |
| <div class="lightbox1-content"> | ||
| <amp-img id="img0" src="https://lh4.googleusercontent.com/-okOlNNHeoOc/VbYyrlFYFII/AAAAAAABYdA/La-3j3c-QQI/w452-h900-p/PANO_20150726_171347%257E2.jpg" width=226 height=450 layout="responsive"></amp-img> | ||
| <div class="lightbox1-content" close> | ||
| <amp-img id="img0" src="https://lh4.googleusercontent.com/-okOlNNHeoOc/VbYyrlFYFII/AAAAAAABYdA/La-3j3c-QQI/w452-h900-p/PANO_20150726_171347%257E2.jpg" width=226 height=450 layout="responsive" on="tap:lightbox1.close"></amp-img> |
There was a problem hiding this comment.
I somewhat overheard but missed the argument why the "close" attribute is needed. Could you explain?
There was a problem hiding this comment.
The spec (md file) says close attr should work , hence the support for the close attr.
UPDATE: Removed now
camelburrito
force-pushed
the
lightbox
branch
5 times, most recently
from
023f1b3 to
6600685
Compare
camelburrito
changed the title
| @@ -706,6 +706,11 @@ class AmpImageLightbox extends AMP.BaseElement { | |||
| this.reset_(); | |||
| this.init_(source); | |||
|
|
|||
| /** @private {function(this:AmpLightbox, Event)}*/ | |||
| this.boundCloseOnEscape_ = event => this.closeOnEscape_(event); | |||
There was a problem hiding this comment.
this.closeOnEscape_.bind(this)
camelburrito
force-pushed
the
lightbox
branch
from
6600685 to
ba96f66
Compare
| @@ -22,7 +22,8 @@ The `amp-lightbox` component allows for a “lightbox” or similar experience - | |||
|
|
|||
| The `amp-lightbox` component defines the child elements that will be displayed in a full-viewport overlay. It is triggered to take up the viewport when the user taps or clicks on an element with `on` attribute that targets `amp-lightbox` element’s `id`. | |||
|
|
|||
| One or more elements within the lightbox can be optionally given a `close` attribute, which when tapped or clicked will close the lightbox. If no element is given a `close` attribute, a tap or click anywhere on the screen will close it. | |||
| Pressing the escape key on the keyboard will close the lightbox. | |||
There was a problem hiding this comment.
Add sub headline for closing the lightbox.
camelburrito
force-pushed
the
lightbox
branch
2 times, most recently
from
58f3b2b to
65052e8
Compare
| @@ -706,6 +706,11 @@ class AmpImageLightbox extends AMP.BaseElement { | |||
| this.reset_(); | |||
| this.init_(source); | |||
|
|
|||
| /** @private {function(this:AmpLightbox, Event)}*/ | |||
There was a problem hiding this comment.
Either AmpImageLightbox or you can simply do {!Function} - the exact signature in this case doesn't matter - you never call it in the code.
|
LGTM with one comment. |
- When ESC key is hit - When a child element with `close` attribute is clicked - When a child element with on attribute is set to "tap:lightbox-id.close" Remove functionality that closes lightbox when tapped anywhere. Image Lightbox(es) should support exiting - When ESC key is hit
camelburrito
force-pushed
the
lightbox
branch
from
65052e8 to
0524e59
Compare
camelburrito
pushed a commit
that referenced
this pull request
Dec 2, 2015
Lightbox(es) should support exiting with escape and x button
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Lightbox(es) should support exiting
closeattribute is clickedRemove functionality that closes lightbox when tapped anywhere.
Image Lightbox(es) should support exiting
This is being tracked in Lightbox(es) should support exiting with escape and x button #342