✨ Proof of concept React BaseElement #24153
Conversation
| * @return {*} | ||
| */ | ||
| render() { | ||
| const props = pick(this.props, ATTRIBUTES_TO_PROPAGATE); |
There was a problem hiding this comment.
Are these not camel-cased?
There was a problem hiding this comment.
It's a local binding at the top of the array.
There was a problem hiding this comment.
I meant: are the attributes->props camel-cased? I.e. should it be "aria-label" or "ariaLabel"?
There was a problem hiding this comment.
Playing around, it appears to work either way... I'm not sure. aria-label should definitely be lowercase, though: https://reactjs.org/docs/dom-elements.html.
| constructor(props) { | ||
| super(props); | ||
|
|
||
| this.state = {prerender: true}; |
There was a problem hiding this comment.
Definitely subject to bikeshedding, but imho, this should be something like "loading=auto|manual", since there might be other reasons to block rendering, besides prerendering.
There was a problem hiding this comment.
Yup, I'm not tied to prerender at all, it just the first name I thought of.
- noprerender - auto-sizes - auto generate src when srcset isn't supported
| props['decoding'] = 'async'; | ||
|
|
||
| const {prerender} = this.state; | ||
| if (prerender && !this.prerenderAllowed_) { |
There was a problem hiding this comment.
I'm curious, why is this.state. prerender and this.prerenderAllowed_ needed? Why not directly check the prop value for noprerender to determine if src and srcset should be deleted?
if (!this.props.noprerender) {
delete props['src'];
delete props['srcset'];
}There was a problem hiding this comment.
Hi!
I think we actually meant to do !prerender || !this.prerenderAllowed. Note that we have to different sources here, one from this.state.prerender (which is outside-of-React props being passed to React) and props.noprerender (which is both the DOM-React and React-React possible values). We use the state to control from the AMP framework, and props to allow the publisher to control behavior.
Also, this PR is stagnant. We started a new attempt at #25969. Feel free to follow along!
This is a proof of concept to support
React/PreactComponents in AMP pages. Once we flesh this out, we can provide a generic class factory to createAMP.BaseElementclasses for each wrapped Component, which can then be registered and used like regular AMP components.A lot of the hard work was already done in
<amp-date-picker>.