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

Refactor amp bento prelim #37181

Closed
wants to merge 4 commits into from

Conversation

kvchari
Copy link
Contributor

@kvchari kvchari commented Dec 10, 2021

partial #36950

  • extract defineBentoElement out of CustomElement class
  • create empty amp-pbe class so that each amp extension can inherit off it. (once all the extensions inherit off AmpPBE, we can safely move method out of PBE into AmpPBE without breaking any behavior).
  • updates 1.0 extension template such that new Amp components will inherit off the new class\
  • minor fixture updates

@kvchari kvchari requested review from alanorozco and samouri and removed request for alanorozco December 10, 2021 21:27
@kvchari kvchari enabled auto-merge (squash) December 10, 2021 21:29
@kvchari kvchari requested a review from dmanek December 10, 2021 22:09
@samouri
Copy link
Member

samouri commented Dec 10, 2021

Can you provide a description?

@@ -3,17 +3,11 @@
<head>
<script src="https://cdn.ampproject.org/bento.js"></script>
<!-- These styles prevent Cumulative Layout Shift on the unupgraded custom element -->
<style>
Copy link
Member

Choose a reason for hiding this comment

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

I'm missing context here, why are these no longer necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we'd decided to recommend consumers ensure that components avoided layout shift by link to our stylesheets rather than copy/paste these styles, but these fixtures were never updated to reflect that. (the link rel is added on line 10)

@@ -6,11 +6,6 @@
async
src="https://cdn.ampproject.org/v0/bento-lightbox-1.0.js"
></script>
<link
Copy link
Member

Choose a reason for hiding this comment

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

Same here, why is this ok to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm didn't mean to remove this, i can revert

@@ -0,0 +1,3 @@
import {PreactBaseElement} from './base-element';

export class AmpPreactBaseElement extends PreactBaseElement {}
Copy link
Member

Choose a reason for hiding this comment

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

nit: not necessary for this PR in particular, but the architecture we are deciding on right now should be written up in a jsdoc

@kvchari
Copy link
Contributor Author

kvchari commented Jan 10, 2022

closing in favor of: #37309

@kvchari kvchari closed this Jan 10, 2022
auto-merge was automatically disabled January 10, 2022 15:52

Pull request was closed

@kvchari kvchari deleted the refactor-amp-bento-prelim branch January 12, 2022 15:46
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.

2 participants