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

amp-story-desktop-one-panel Background blur MVP #34974

Merged
merged 19 commits into from
Jul 2, 2021

Conversation

processprocess
Copy link
Contributor

@processprocess processprocess commented Jun 22, 2021

Contributes to #34764
Fixes #34960 #34967 #34962 #34959

An AmpStoryBackgroundBlur class with MVP methods.

  • Renders a 3x3 canvas
  • Fades the new image in on page transition
  • If no image is found, fades to black

Demo

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jun 22, 2021

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

extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/background-blur.js

extensions/amp-story/1.0/amp-story-background-blur.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story-background-blur.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story-background-blur.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story-background-blur.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story-background-blur.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story-background-blur.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story-background-blur.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story-background-blur.js Outdated Show resolved Hide resolved
* @return {?Element} An img element or null.
*/
getBiggestImage_(pageElement) {
const getSize = (el) => el && el.offsetWidth * el.offsetHeight;
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this solution of getting the biggest img!

The one worry I have is offsetWidth/Heigh triggering relayout/paint.
To work around it, I suggest we query the amp-img instead of the img, and use getLayoutBox() that returns cached measurements from the AMP runtime.

Copy link
Contributor Author

@processprocess processprocess Jun 25, 2021

Choose a reason for hiding this comment

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

Looks like getLayoutBox is being deprecated and checks wont pass with it:
https://app.circleci.com/pipelines/github/ampproject/amphtml/12123/workflows/fbc193e8-aff0-49b9-89d5-081efcc40a61/jobs/193397?invite=true#step-111-99

I2D:
#31540

I wonder what a good alternative would be.

@processprocess
Copy link
Contributor Author

cc @rsimha for OWNERS on forbidden-terms.js.
An alternative could be a resize-observer helper that observes each amp-img measured.
I'm not sure if we want to set that up. getLayoutBox is very helpful in cases like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

amp-story-desktop-one-panel blurred background, scaffold class
3 participants