Skip to content
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

Add basic documentation for amp-lightbox-gallery #13272

Merged
merged 5 commits into from
Feb 6, 2018

Conversation

cathyxz
Copy link
Contributor

@cathyxz cathyxz commented Feb 5, 2018

Basic documentation for an experimental component <amp-lightbox-gallery>. The API for styling, thumbnails, and supported elements is still in progress, hence there is only basic usage under an experiment flag.

@cathyxz cathyxz requested review from a user and aghassemi February 5, 2018 21:52
@cathyxz cathyxz closed this Feb 5, 2018
@cathyxz cathyxz deleted the feature/lightbox-docs branch February 5, 2018 22:07
@cathyxz cathyxz restored the feature/lightbox-docs branch February 5, 2018 22:07
@cathyxz cathyxz reopened this Feb 5, 2018
## Behavior
All you need to do is to include the 'lightbox' attribute on an `<amp-img>` or `<amp-carousel>`. A typical usage looks like this:

### Lightbox with AMP-IMG
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowecase amp-img

</table>

## Behavior
All you need to do is to include the 'lightbox' attribute on an `<amp-img>` or `<amp-carousel>`. A typical usage looks like this:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...is to include the extension script and put the lightbox attribute...

<amp-img src="image2" width=200 height=100 lightbox></amp-img>
```

The `<amp-lightbox-gallery>` extension automatically inserts an invisible `<amp-lightbox-gallery>` component (with the id 'amp-lightbox-gallery') into the dom.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed, it is very internal implementation details.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

</amp-carousel>
```

`<amp-carousel>` is considered a lightbox container item. To process an `<amp-carousel>` with the `lightbox` attribute, `<amp-lightbox-gallery>` extension will iterate all of the `<amp-carousel>`'s children, add the `lightbox` attribute and a tap-handler to each child.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's mention the carousel is kept in sync with lightbox gallery


### Captions
Lightbox optionally allows the user to specify a caption for each element. These fields are automatically read and displayed by `<amp-lightbox-gallery>` in the following order of priority:
- figcaption (if the lightboxed element is a figure)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if lightboxed element is a child of a figure

<amp-img src="image2" width=200 height=100 lightbox></amp-img>
```

The `<amp-lightbox-gallery>` extension automatically inserts an invisible `<amp-lightbox-gallery>` component (with the id 'amp-lightbox-gallery') into the dom.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

- `alt`
- `aria-label`
- `aria-labelledby`

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest a code snippet example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

### Lightbox with `<amp-img>`

```html
<amp-img src="image1" width=200 height=100 lightbox></amp-img>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In code samples, attribute values are enclosed in double quotes (e.g., "width="200"). Please apply that standard to your code sample here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


## Overview

The `amp-lightbox-gallery` component provides a "lightbox” experience for AMP components (e.g., `amp-img`, `amp-carousel`). When the user interacts with the AMP element, a UI component expands to fill the viewport until it is closed by the user. Currently, only images are supported.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworded this. Is it only for AMP components or can it be applied to HTML elements too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're not quite sure yet. We're currently supporting things on a whitelisted basis, which only includes amp-img and amp-carousel. We should be able to support divs, but my understanding is that AMP doesn't really allow very many raw html elements? E.g. and

I'm open to changing the wording on this so long as we make our support guarantees explicit until we're ready.

@cathyxz cathyxz merged commit 14e352a into ampproject:master Feb 6, 2018
@cathyxz cathyxz deleted the feature/lightbox-docs branch February 16, 2018 13:58
protonate pushed a commit to protonate/amphtml that referenced this pull request Feb 26, 2018
* Add basic documentation for amp-lightbox-gallery

* Update docs to address comments

* Wording cleanup

* Add documentation for descriptions

* Update amp-lightbox-gallery.md
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
* Add basic documentation for amp-lightbox-gallery

* Update docs to address comments

* Wording cleanup

* Add documentation for descriptions

* Update amp-lightbox-gallery.md
protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018
* Add basic documentation for amp-lightbox-gallery

* Update docs to address comments

* Wording cleanup

* Add documentation for descriptions

* Update amp-lightbox-gallery.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants