Skip to content

Swipe down to close the bookend.#23761

Merged
gmajoulet merged 5 commits intoampproject:masterfrom
gmajoulet:draggable_bookend
Aug 7, 2019
Merged

Swipe down to close the bookend.#23761
gmajoulet merged 5 commits intoampproject:masterfrom
gmajoulet:draggable_bookend

Conversation

@gmajoulet
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet commented Aug 6, 2019

Re-using the new draggable-drawer abstract class extracted from page attachments to enable swiping down to close the bookend.

Animations + scrolling logic is now handled by the draggable-drawer and removed from the bookend implementation.

  • Mobile: swipe down interactions + small handle on top of the bookend
  • Desktop: no-op

Demo here

Part 2/2
Fixes #13833

@gmajoulet gmajoulet requested review from Enriqe and newmuis August 6, 2019 23:28
overflow: hidden !important;
}

/** Bookend overrides. */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should these go here? Doesn't this break encapsulation a little bit, that subclass styles are defined in the superclass?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're 100% right

The issue is that the amp-story-bookend.css code goes into the bookend shadow DOM. We could have this CSS block into the main amp-story.css file, or keep it in the draggable-drawer.css file. I picked the latter, but I don't feel strongly. What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SGTM. In theory we could add a new bookend-specific CSS that is the "outer" styles of the bookend, but that's just a lot of overhead to maintain, so... 🤷‍♂


this.state_ = DrawerState.OPEN;

this.storeService_.dispatch(Action.TOGGLE_SYSTEM_UI_IS_VISIBLE, false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we not want this behavior anymore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When the bookend is open, the system layer stays visible with a ~0.3 opacity. I tried to go for an identical design :)

@newmuis
Copy link
Copy Markdown
Contributor

newmuis commented Aug 6, 2019

This experience feels super smooth!

@gmajoulet
Copy link
Copy Markdown
Contributor Author

Travis should go green this time, PTAL :)

@gmajoulet gmajoulet merged commit e0ab442 into ampproject:master Aug 7, 2019
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.

Add swipe down gesture to close the amp-story bookend on mobile

3 participants