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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 [amp story] Pagination forward button visibility state #37098

Merged
merged 12 commits into from
Dec 23, 2021

Conversation

processprocess
Copy link
Contributor

@processprocess processprocess commented Dec 2, 2021

When the attachment is opened, the icon in the forward button disappear when the HIDDEN state is applied.
This is due to the SVG being applied through the state class.

Screen Shot 2021-12-02 at 1 14 25 PM

This PR:

  • Removes HIDDEN from state, considering it a modifier rather than state.
  • Implements a function to toggle disabled attribute and hidden class.
  • Adds a one-panel story to /examples

demo one panel
demo supports landscape

Background:
Prior to the desktop one-panel UI the pagination buttons disappeared when the attachment opened.
In the new UI the buttons fade to .1 opacity.
This was not noticed when developing the desktop one-panel UI. This may have been due to the attachment demo being in landscape mode. A one-panel story has been added /examples to help assist with developing for attachments in both UI.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Dec 2, 2021

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/pagination-buttons.js

extensions/amp-story/1.0/pagination-buttons.js Outdated Show resolved Hide resolved
/** @private @const {!Element} */
this.buttonElement_ = devAssert(this.element.firstElementChild);
/** @const {!Element} */
this.buttonElement = devAssert(this.element.firstElementChild);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably keep this property private since it's not being used elsewhere and the difference between paginationButton.element and paginationButton.buttonElement is not immediately clear

extensions/amp-story/1.0/pagination-buttons.js Outdated Show resolved Hide resolved
toggleEnabled_(paginationButton, isVisible) {
const {buttonElement, element} = paginationButton;
element?.classList.toggle('i-amphtml-story-button-hidden', !isVisible);
buttonElement?.toggleAttribute('disabled', !isVisible);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the buttonElement need to be disabled if element is already hidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does since the element is still visible, it just has a lowered opacity (I updated the naming). Button elements will receive focus unless disabled is explicitly set on them.

extensions/amp-story/1.0/pagination-buttons.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/pagination-buttons.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/pagination-buttons.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

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

Added a couple of nits.

Also note to myself (for possible cleanup in the future): we could reduce the complexity of this code if we don't have the nested elements and just have one HTML button element for each, I bet we can simplify a lot of the styles (eg: use [disabled] instead of i-amphtml-story-button-hidden, get rid of class ..-button-move, etc).

extensions/amp-story/1.0/pagination-buttons.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/pagination-buttons.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/pagination-buttons.js Outdated Show resolved Hide resolved
@processprocess processprocess merged commit 1c7b936 into ampproject:main Dec 23, 2021
wg-stories Sprint automation moved this from In progress to Done Dec 23, 2021
@processprocess processprocess deleted the buttons branch December 23, 2021 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants