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.

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

Jump to bottom

🚀 🚀 🚀 amp-carousel ssr: allow render without JS 🚀 🚀 🚀 #37646

Merged
merged 10 commits into from Feb 16, 2022
Merged

🚀 🚀 🚀 amp-carousel ssr: allow render without JS 🚀 🚀 🚀 #37646

merged 10 commits into from Feb 16, 2022

Conversation

samouri
Copy link
Member

@samouri samouri commented Feb 11, 2022 •
edited

summary
Server-rendering AMP Components should allow us to have UI visible without blocking on JS executaion.
Via disabling JS locally, I was surprised to see that this wasn't the case yet.

We still have two issues stopping <amp-carousel> from rendering without JS:

  1. Loading extension CSS is still bundled with JS execution. We need to add a <style> link to the CSS to the <head>.
  2. The application of class names which unhide the amp-carousel take place in layoutCallback instead of buildDom. In this case it is the addition of i-amphtml-element and i-amphtml-slide-item-show.

I solve (2) in this PR, and am working on (1) separately.

🚀 🚀 🚀 results 🚀 🚀 🚀
For webpages with their LCP Element in the amp-carouse, we can expect a ~40% improvement to LCP. source

lcp-improvement

@samouri samouri force-pushed the init-display branch 2 times, most recently from 6cf0678 to 5e6d167 Compare February 11, 2022 20:55
@samouri samouri changed the title amp-carousel ssr: allow render without JS 🚀 🚀 🚀 amp-carousel ssr: allow render without JS 🚀 🚀 🚀 Feb 14, 2022
@samouri samouri self-assigned this Feb 14, 2022
@samouri samouri mentioned this pull request Feb 14, 2022
Closed
rcebulko
rcebulko reviewed Feb 15, 2022
View reviewed changes
Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

LGTM so far w/ 1 minor tweak but in draft so will hold off approving

extensions/amp-carousel/0.1/slidescroll.js Outdated Show resolved Hide resolved
@samouri samouri marked this pull request as ready for review February 16, 2022 00:21
@amp-owners-bot
Copy link

amp-owners-bot bot commented Feb 16, 2022 •
edited

Hey @Jiaming-X, @jeffkaufman! These files were changed:

extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js

Hey @jridgewell! These files were changed:

src/core/dom/style.js

@samouri
Copy link
Member Author

samouri commented Feb 16, 2022

for the reviewer
There are three notable changes in this PR:

  1. All SSRed elements will now have the class i-amphtml-element
  2. All slidescroll <amp-carousel>s will have their first slide start out visible
  3. setStyle now exclusively uses setProperty method instead of ever falling back to assignment on style keys. This is accomplished with a new function, camelCaseToHyphenCase

jridgewell
jridgewell requested changes Feb 16, 2022
View reviewed changes
extensions/amp-carousel/0.1/build-dom.js
@@ -229,6 +231,8 @@ function buildSlideScrollCarousel(element) {
slidesContainer.appendChild(slideWrapper);
slideWrappers.push(slideWrapper);
});
// Initialize the first slide to be shown.
slideWrappers[0]?.classList.add(ClassNames.SLIDES_ITEM_SHOW);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Move this into the forEach above? Also, why isn't it a map…

Copy link
Member Author

@samouri samouri Feb 16, 2022 •
edited

Choose a reason for hiding this comment

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

I'm confused. Why should this be part of the forEach (it should only occur to a single elem) and what should be a map?

Copy link
Member Author

@samouri samouri Feb 16, 2022 •
edited

Choose a reason for hiding this comment

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

Oh I see, instead of arr.forEach it could be arr.map.
Agreed!

Mind if I punt on that to a small PR tomorrow? (unless you have more feedback and I need to rerun CI anyway)

src/core/dom/style.js Show resolved Hide resolved
src/core/dom/style.js Outdated
* @return {string} hyphen-cased string
*/
export function camelCaseToHyphenCase(camelCase) {
return camelCase.replace(/[A-Z]/g, (match) => '-' + match.toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

webkitFoo can be lowercase, but still needs a leading dash.

Copy link
Member Author

@samouri samouri Feb 16, 2022 •
edited

Choose a reason for hiding this comment

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

Great find!
This seems tricky to handle for opera

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it

@samouri
Copy link
Member Author

samouri commented Feb 16, 2022

There seem to be a long-tail of test cases that create a fake style which of course now needs to have a setProperty method on it. Hopefully caught them all.

@samouri
Copy link
Member Author

samouri commented Feb 16, 2022

@jridgewell: ready for a rereview

@jridgewell jridgewell merged commit a50f171 into ampproject:main Feb 16, 2022
@samouri samouri deleted the init-display branch February 16, 2022 02:46
@samouri samouri mentioned this pull request Feb 16, 2022
Closed
rileyajones pushed a commit that referenced this pull request Feb 18, 2022
@samouri @rileyajones
🚀 🚀 🚀 amp-carousel ssr: allow render without JS 🚀 🚀 🚀 (#37646)
e677250 
* amp-carousel ssr: allow render without JS

* lint. scrollslide working

* rcebulko feedback

* further fixes

* hyphen-case setProperty and remove Set

* fix test

* add an extra test

* moar fix

* solve lowercase vendor bug.

* even moar test fixes

(cherry picked from commit a50f171)
ampprojectbot pushed a commit that referenced this pull request Feb 18, 2022
@samouri @siddharthlshah
🚀 🚀 🚀 amp-carousel ssr: allow render without JS 🚀 🚀 🚀 (#37646)
3116a3c 
* amp-carousel ssr: allow render without JS

* lint. scrollslide working

* rcebulko feedback

* further fixes

* hyphen-case setProperty and remove Set

* fix test

* add an extra test

* moar fix

* solve lowercase vendor bug.

* even moar test fixes

(cherry picked from commit a50f171)
@ampprojectbot ampprojectbot mentioned this pull request Feb 22, 2022
Closed
@samouri samouri mentioned this pull request Feb 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcebulko rcebulko rcebulko left review comments

@jridgewell jridgewell jridgewell approved these changes

Assignees

@samouri samouri

Labels
Component: amp-carousel PR use: In LTS PR use: In Stable WG: performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@samouri @jridgewell @rcebulko @ampprojectbot