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

UX Polish for Lightbox #13147

Merged
merged 2 commits into from Jan 30, 2018
Merged

UX Polish for Lightbox #13147

merged 2 commits into from Jan 30, 2018

Conversation

cathyxz
Copy link
Contributor

@cathyxz cathyxz commented Jan 30, 2018

This is a summary PR for code-trivial UX changes that we decided in the last round of QA. Anything that still requires discussion will happen after UX OH today.

  • Add cursor: pointer to images in lightbox gallery.
  • Add looping to lightbox.

Copy link
Contributor

@cvializ cvializ left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -293,6 +293,7 @@ export class AmpLightboxViewer extends AMP.BaseElement {
this.carousel_ = this.win.document.createElement('amp-carousel');
this.carousel_.setAttribute('type', 'slides');
this.carousel_.setAttribute('layout', 'fill');
this.carousel_.setAttribute('loop', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

🥇

@cathyxz cathyxz merged commit f20b02e into ampproject:master Jan 30, 2018
@cathyxz cathyxz deleted the feature/ui-polish branch January 30, 2018 20:41
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
* Make lightbox carousels loop

* Add cursor: pointer to lightbox gallery images
protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018
* Make lightbox carousels loop

* Add cursor: pointer to lightbox gallery images
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants