♻️ amp-next-page v2 host page visibility + minor refactor
#26778
Conversation
… into amp-next-page-v2-bug-fix
|
@alanorozco ping on this |
| * @param {!VisibilityObserverEntry} entry | ||
| * @return {?ViewportRelativePos} | ||
| */ | ||
| getRelativePosFromSentinel(entry) { |
There was a problem hiding this comment.
I'm confused about state flow.
Why is the concept of sentinels spilling to this level? Should this be localized only when you need a sentinel?
There was a problem hiding this comment.
Sentinel is actually the general case, the host page is a special case because we don't want to inject sentinel elements into the body of the document so we rely on the amp-next-page element position for calculations
|
Added @jridgewell to review the minor |
wassgha
force-pushed
the
amp-next-page-host-visibility
branch
from
a54d716 to
d8e3603
Compare
| @@ -224,6 +219,7 @@ export class Page { | |||
| if ( | |||
| this.state_ === PageState.INSERTED || | |||
| this.state_ === PageState.FETCHING || | |||
| this.state_ === PageState.LOADED || | |||
There was a problem hiding this comment.
Reading, it seems like they were allowed to refetch after it was loaded, and we're disallowing that now. Was it intentionally done before?
There was a problem hiding this comment.
It was unintentionally forgotten. We don't want a refetch during loading because it can cause duplicate insertion. After the LOADED state, it would either be FAILED or INSERTED and that's when we'd want to refetch or not.
| @@ -156,8 +169,106 @@ export default class VisibilityObserver { | |||
| constructor(ampdoc) { | |||
There was a problem hiding this comment.
Dima is working on a IntersectionObserver polyfill, which will make InOb available everywhere. Can this be easily extended to InObs?
There was a problem hiding this comment.
If it has the same InsersectionObserver behavior then no. The issue is when the element is bigger than the viewport, the IntersectionObserver stops serving updates whereas here we need to still be updated, hence this polyfill.
…ct#26778) * Added scroll-bound animation to amp-next-page * Smoother animation * Fix issue with tests * Type checking fix * Type checking fix * Refactor visibility observer and adds visibility observing for the host page * Reverted changes from old PR * Second refactor * Fixed tests * Suggested changes * Requested changes + title fixes * Added missing check * Review feedback
|
@wassgha when these changes are live? |
|
@wassgha are these changes live? |
Closes #27635
Closes #26628
Changes