Skip to content

🐛 [Story performance] Disabling animations on initial page when first loaded #36668

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.

Sign up for GitHub

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

Closed
mszylkowski wants to merge 6 commits into from
+42 −27
Closed

🐛 [Story performance] Disabling animations on initial page when first loaded #36668

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9e73016
Disabling animations when first loaded only
mszylkowski Oct 28, 2021
df7609a
Remove console.log
mszylkowski Oct 29, 2021
a87b850
Remove animations on non visited, initial page
mszylkowski Nov 3, 2021
f9e3ef4
Added tests
mszylkowski Nov 3, 2021
a2637df
Merge branch 'noanimation_firstpage' of github.com:mszylkowski/amphtm…
mszylkowski Nov 3, 2021
eae03ca
Revert test indentation
mszylkowski Nov 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove animations on non visited, initial page
  • Loading branch information
@mszylkowski
mszylkowski committed Nov 3, 2021
commit a87b850f1302703ad911dd674b01b50164abd5bb
24 changes: 7 additions & 17 deletions extensions/amp-story/1.0/amp-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,7 @@ const Attributes = {
RETURN_TO: 'i-amphtml-return-to',
STANDALONE: 'standalone',
SUPPORTS_LANDSCAPE: 'supports-landscape',
// Attributes that desktop css looks for to decide where pages will be placed
VISITED: 'i-amphtml-visited', // stacked offscreen to left
VISITED: 'i-amphtml-visited',
};

/**
Expand Down Expand Up @@ -360,6 +359,7 @@ export class AmpStory extends AMP.BaseElement {
`amp-story-page#${escapeCssSelectorIdent(pageId)}`
);
page.setAttribute('active', '');
page.setAttribute('i-amphtml-initial', '');
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet Nov 8, 2021

Choose a reason for hiding this comment

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

This attribute is very broad and could mean literally anything, please be a bit more specific and/or consistent with how things are named in the JavaScript to make it easier

}

this.initializeStyles_();
Expand Down Expand Up @@ -1415,12 +1415,6 @@ export class AmpStory extends AMP.BaseElement {
if (oldPage) {
oldPage.setState(PageState.NOT_ACTIVE);

// Indication to know where to display the page on the desktop
// ribbon-like animation.
this.getPageIndex(oldPage) < pageIndex
? setAttributeInMutate(oldPage, Attributes.VISITED)
: removeAttributeInMutate(oldPage, Attributes.VISITED);

if (oldPage.isAd()) {
this.storeService_.dispatch(
Action.SET_ADVANCEMENT_MODE,
Expand Down Expand Up @@ -1474,6 +1468,10 @@ export class AmpStory extends AMP.BaseElement {
this.systemLayer_.setDeveloperLogContextString(
this.activePage_.element.id
);

if (oldPage) {
setAttributeInMutate(oldPage, Attributes.VISITED);
}
},
];

Expand Down Expand Up @@ -2368,7 +2366,7 @@ export class AmpStory extends AMP.BaseElement {
/** @private */
replay_() {
this.storeService_.dispatch(Action.SET_NAVIGATION_PATH, []);
const switchPromise = this.switchTo_(
this.switchTo_(
dev().assertElement(this.pages_[0].element).id,
NavigationDirection.NEXT
);
Expand All @@ -2377,14 +2375,6 @@ export class AmpStory extends AMP.BaseElement {
this.pages_[0].setState(PageState.NOT_ACTIVE);
this.pages_[0].setState(PageState.PLAYING);
}

// Reset all pages so that they are offscreen to right instead of left in
// desktop view.
switchPromise.then(() => {
this.pages_.forEach((page) =>
removeAttributeInMutate(page, Attributes.VISITED)
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet Nov 8, 2021

Choose a reason for hiding this comment

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

replay_() is really trying to reset everything, including this, like a page reload. We should still remove the attributes

Copy link
Copy Markdown
Contributor Author

@mszylkowski mszylkowski Nov 9, 2021

Choose a reason for hiding this comment

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

In this PR I want to make the removal of animations more targeted:

  • Current: first page of each story (independently of whether it was visited before or not).
  • New: initial page of the story on the first visit of the page.

This means that when the story is replayed we don't need to skip the animations, especially because we're not on the initial page any more on some cases.

However, I understand your point so I'll re-implement this feature with better semantics (and probably remove this attribute altogether).

);
});
}

/**
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-story/1.0/animation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const ANIMATE_IN_TIMING_FUNCTION_ATTRIBUTE_NAME = 'animate-in-timing-function';
const ANIMATABLE_ELEMENTS_SELECTOR = `[${ANIMATE_IN_ATTRIBUTE_NAME}]`;
/** @const {string} selector for disabling animations on first page when first loaded */
const DISABLE_ANIMATIONS_FIRST_PAGE_SELECTOR =
'amp-story-page:first-of-type:not([i-amphtml-visited])';
'[i-amphtml-initial]:not([i-amphtml-visited])';

/** @const {string} */
const DEFAULT_EASING = 'cubic-bezier(0.4, 0.0, 0.2, 1)';
Expand Down
13 changes: 0 additions & 13 deletions extensions/amp-story/1.0/test/test-amp-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,19 +789,6 @@ describes.realWin(
});
});

it('should add previous visited attribute', async () => {
env.sandbox
.stub(utils, 'setAttributeInMutate')
.callsFake((el, attr) => el.element.setAttribute(attr, ''));

await createStoryWithPages(2, ['page-0', 'page-1']);
const pages = story.element.querySelectorAll('amp-story-page');
const page0 = pages[0];
await story.layoutCallback();
await story.switchTo_('page-1');
expect(page0.hasAttribute('i-amphtml-visited')).to.be.true;
});

describe('amp-story audio', () => {
it('should register and preload the background audio', async () => {
const src = 'https://example.com/foo.mp3';
Expand Down