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

Support bindings on non-AMP elements #895

Open
wants to merge 1 commit into
base: ampstart-2.0
Choose a base branch
from
Open

Support bindings on non-AMP elements #895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions lib/next-amp-demo/pages/components/AmpBind.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,31 @@
*/

import React from 'react';
import {AmpImg, AmpCarousel} from '@ampproject/toolbox-next-amp/src-gen';
import {AmpBind, AmpImg, AmpCarousel} from '@ampproject/toolbox-next-amp';
export const config = {amp: true};

export default () => (
<>
<h1>amp-bind</h1>

<h2>Binding AMP Elements</h2>
<AmpCarousel bindSlide='slide' width='400' height='300' type='slides'>
<AmpImg src='https://picsum.photos/id/231/400/300' layout='fill'></AmpImg>
<AmpImg src='https://picsum.photos/id/232/400/300' layout='fill'></AmpImg>
<AmpImg src='https://picsum.photos/id/233/400/300' layout='fill'></AmpImg>
<AmpImg src='https://picsum.photos/id/231/400/300' layout='fill' />
<AmpImg src='https://picsum.photos/id/232/400/300' layout='fill' />
<AmpImg src='https://picsum.photos/id/233/400/300' layout='fill' />
</AmpCarousel>
<button
on='tap:AMP.setState({
slide: slide != undefined ? (slide + 1) % 3 : 0
})'
slide: slide != undefined ? (slide + 1) % 3 : 0
})'
>
Next slide
</button>

<h2>Binding non-AMP Elements</h2>
<AmpBind bindText="greeting || '' ">
<div />
</AmpBind>
<button on="tap:AMP.setState({ greeting: 'hello world' })">Say hello</button>
</>
);
14 changes: 10 additions & 4 deletions lib/next-amp-demo/test/out/components/AmpBind.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,21 @@
<script async src="https://cdn.ampproject.org/v0.js"></script>
<script
async
custom-element="amp-bind"
src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"
custom-element="amp-carousel"
src="https://cdn.ampproject.org/v0/amp-carousel-0.2.js"
></script>
<script
async
custom-element="amp-carousel"
src="https://cdn.ampproject.org/v0/amp-carousel-0.2.js"
custom-element="amp-bind"
src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"
></script>
<style amp-custom></style>
<link rel="canonical" href="/components/AmpBind" />
</head>
<body>
<!-- __NEXT_DATA__ -->
<h1>amp-bind</h1>
<h2>Binding AMP Elements</h2>
<amp-carousel
width="400"
height="300"
Expand Down Expand Up @@ -56,5 +57,10 @@
>
Next slide
</button>
<h2>Binding non-AMP Elements</h2>
<div data-amp-bind-text="greeting || '' "></div>
<button on="tap:AMP.setState({ greeting: 'hello world' })">
Say hello
</button>
</body>
</html>
22 changes: 18 additions & 4 deletions lib/next-amp/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,14 +215,28 @@ Better than Option 1, but makes things more complicated.

### amp-bind

For `amp-bind` expressions, the `amp-bind` extension is automatically imported using the same approach as for `amp-fx-collection`. The following will work out-of-the-box:
AMP bindings can be expressed using the the `bindX` short-form instead of `data-amp-bind-x`. A nice
side-effect is that most JS IDEs will offer component specific code-completion once you've
typed `bind`.

Here is an example binding an `AmpImg`'s `src` attribute via `bindSrc`:

```
<AmpImg width="300" height="200" src="image.png" bindSrc="imageUrl"/>
<button on="tap:AMP.setState({ imageUrl: 'anotherImage.png' })">Change image</button>
```

Unfortunately, this doesn't work for non-AMP tags such as `div` or `input`. In this case you need to
wrap the element inside the `AmpBind` component:

```
<div data-amp-bind-text="hello"/>
<button on="tap:AMP.setState({ "hello": "world" })">
<AmpBind bindText="greeting">
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a pity extra markup is required find bind attributes, but if there is no alternative supported by Next.js then a wrapper element names sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not happy about it either. I see two other options:

  1. HTML Element specific components:
    <AmpDiv bindText/>
    This would have the added benefit of element specific auto completion. But it's probably more confusing than it helps.
  2. Add bindX types to the HTML elements and rename the attributes in AMP Optimizer. This would work best, but the downside is that it means we have to move Next.js specific features into AMP Optimizer.

<div />
</AmpBind>
<button on="tap:AMP.setState({ greeting: 'hello world' })">Say hello</button>
```

State can be initialized via:
`amp-state` can be initialized via:

```
<AmpState id="myState">{{
Expand Down
5 changes: 5 additions & 0 deletions lib/next-amp/build/generate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ const loadAmp = require('./amp');
const generateTypes = require('./generateTypes.js');
const generateComponents = require('./generateComponents');
const generateVersionMapping = require('./generateVersionMapping');
const generateBindProps = require('./generateAmpBindProps');

const prettierConfig = require('../../../prettier.config.js');

const config = {
importHelper: 'AmpCustomElement.tsx',
componentWrapper: 'index.tsx',
versionMapping: 'versionMapping.ts',
bindProps: 'AmpBindProps.ts',
typeDefinition: 'amp.d.tsx',
dist: join(__dirname, '../src-gen/'),
lincenseHeader: `/**
Expand Down Expand Up @@ -63,6 +65,9 @@ async function build() {

const versionMapping = generateVersionMapping(config, amp);
writeFile(config.versionMapping, versionMapping);

const bindProps = generateBindProps(config, amp);
writeFile(config.bindProps, bindProps);
}

function writeFile(name, content) {
Expand Down
15 changes: 15 additions & 0 deletions lib/next-amp/build/generateAmpBindProps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module.exports = (config, amp) => {
const bindAttrs = Array.from(
new Set(amp.tags.map(tag => tag.bindAttrs.map(a => `${a.name}?: string`)).flat()).values()
);
return `${config.lincenseHeader}

import * as React from 'react';

type AmpBindProps = {
children: React.ReactElement;
${bindAttrs.join(';\n')}
};
export default AmpBindProps;
`;
};
41 changes: 41 additions & 0 deletions lib/next-amp/src/components/AmpBind.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Copyright 2019 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as React from 'react';
import {AmpIncludeCustomElement} from '../../src-gen/';
import AmpBindProps from '../../src-gen/AmpBindProps';

/**
* Renders an amp-state element, by either adding local state via `value`
* or remote state via the `src` property.
*/
const AmpBind: React.FunctionComponent<AmpBindProps> = ({children, ...rest}) => {
const newProps = {};
Object.keys(rest).forEach(key => {
if (/^bind[A-Z]/g.test(key)) {
const newKey = `data-amp-bind-${key.substring(4).toLowerCase()}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to lower case the whole string or just the first character of the string? I know standard HTML elements won't have hyphenated names, but just felt a little risking assuming nothing else ever will.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The React toString serializer will automatically lowercase attribute names. So I think it's fine to lowercase by default.

newProps[newKey] = rest[key];
}
});
return (
<>
<AmpIncludeCustomElement name='amp-bind' />
{React.cloneElement(children, newProps)}
</>
);
};

export default AmpBind;