Skip to content

Commit

Permalink
♻️ amp-story: Replace htmlFor and simple-template with JSX (#36682
Browse files Browse the repository at this point in the history 
)

Removes usages of `htmlFor` and `simple-template` in `extensions/amp-story/**`.

It also ports `createElementWithAttributes` to JSX and removes `addAttributesToElement` for the sake of keeping the bundle size as small as possible.

Caveats: 

- We prioritize safety. Template structure is left intact and is therefore sub-optimal given JSX. Bundle size may increase by a small amount. Renderers can be later refactored to take advantage of JSX features, and improve readability and output size.

- Does not update tests that use `htmlFor`, since they require a context node that's not provided using JSX.
  • Loading branch information
@alanorozco
alanorozco committed Nov 2, 2021
1 parent 7bd4192 commit b8123ef
Show file tree
Hide file tree
Showing 24 changed files with 566 additions and 968 deletions.
48 changes: 24 additions & 24 deletions extensions/amp-story/1.0/amp-story-access.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as Preact from '#core/dom/jsx';
import {
Action,
StateProperty,
Expand All @@ -8,7 +9,6 @@ import {closest} from '#core/dom/query';
import {copyChildren, removeChildren} from '#core/dom';
import {dev, user} from '#utils/log';
import {getStoryAttributeSrc} from './utils';
import {htmlFor} from '#core/dom/static-template';
import {isArray, isObject} from '#core/types';
import {parseJson} from '#core/types/object/json';
import {setImportantStyles} from '#core/dom/style';
Expand All @@ -26,37 +26,37 @@ export const Type = {

/**
* Story access blocking type template.
* @param {!Element} element
* @return {!Element}
*/
const getBlockingTemplate = (element) => {
return htmlFor(element)`
<div class="i-amphtml-story-access-overflow">
<div class="i-amphtml-story-access-container">
<div class="i-amphtml-story-access-header">
<div class="i-amphtml-story-access-logo"></div>
</div>
<div class="i-amphtml-story-access-content"></div>
const renderBlockingElement = () => {
return (
<div class="i-amphtml-story-access-overflow">
<div class="i-amphtml-story-access-container">
<div class="i-amphtml-story-access-header">
<div class="i-amphtml-story-access-logo"></div>
</div>
</div>`;
<div class="i-amphtml-story-access-content"></div>
</div>
</div>
);
};

/**
* Story access notification type template.
* @param {!Element} element
* @return {!Element}
*/
const getNotificationTemplate = (element) => {
return htmlFor(element)`
<div class="i-amphtml-story-access-overflow">
<div class="i-amphtml-story-access-container">
<div class="i-amphtml-story-access-content">
<span class="i-amphtml-story-access-close-button" role="button">
&times;
</span>
</div>
const renderNotificationElement = () => {
return (
<div class="i-amphtml-story-access-overflow">
<div class="i-amphtml-story-access-container">
<div class="i-amphtml-story-access-content">
<span class="i-amphtml-story-access-close-button" role="button">
&times;
</span>
</div>
</div>`;
</div>
</div>
);
};

/**
Expand Down Expand Up @@ -198,7 +198,7 @@ export class AmpStoryAccess extends AMP.BaseElement {
renderDrawerEl_() {
switch (this.getType_()) {
case Type.BLOCKING:
const drawerEl = getBlockingTemplate(this.element);
const drawerEl = renderBlockingElement();

const logoSrc = getStoryAttributeSrc(
this.element,
Expand All @@ -216,7 +216,7 @@ export class AmpStoryAccess extends AMP.BaseElement {
return drawerEl;
break;
case Type.NOTIFICATION:
return getNotificationTemplate(this.element);
return renderNotificationElement();
break;
default:
user().error(
Expand Down
10 changes: 5 additions & 5 deletions extensions/amp-story/1.0/amp-story-affiliate-link.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as Preact from '#core/dom/jsx';
/**
* @fileoverview Affiliate link component that expands when clicked.
*/
Expand All @@ -6,7 +7,6 @@ import {Services} from '#service';
import {StateProperty, getStoreService} from './amp-story-store-service';
import {StoryAnalyticsEvent, getAnalyticsService} from './story-analytics';
import {getAmpdoc} from '../../../src/service-helpers';
import {htmlFor} from '#core/dom/static-template';

/**
* Links that are affiliate links.
Expand Down Expand Up @@ -116,14 +116,15 @@ export class AmpStoryAffiliateLink {
* @private
*/
addIconElement_() {
const iconEl = htmlFor(this.element_)`
const iconEl = (
<div class="i-amphtml-story-affiliate-link-circle">
<i class="i-amphtml-story-affiliate-link-icon"></i>
<div class="i-amphtml-story-reset i-amphtml-hidden">
<span class="i-amphtml-story-affiliate-link-text" hidden></span>
<i class="i-amphtml-story-affiliate-link-launch" hidden></i>
</div>
</div>`;
</div>
);
this.element_.appendChild(iconEl);
}

Expand Down Expand Up @@ -157,8 +158,7 @@ export class AmpStoryAffiliateLink {
* @private
*/
addPulseElement_() {
const pulseEl = htmlFor(this.element_)`
<div class="i-amphtml-story-affiliate-link-pulse"></div>`;
const pulseEl = <div class="i-amphtml-story-affiliate-link-pulse"></div>;
this.element_.appendChild(pulseEl);
}
}
191 changes: 73 additions & 118 deletions extensions/amp-story/1.0/amp-story-consent.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as Preact from '#core/dom/jsx';
import {
Action,
StateProperty,
Expand All @@ -23,13 +24,12 @@ import {
triggerClickFromLightDom,
} from './utils';
import {dev, user, userAssert} from '#utils/log';
import {dict} from '#core/types/object';
import {isArray} from '#core/types';
import {isJsonScriptTag} from '#core/dom';

import {parseJson} from '#core/types/object/json';
import {renderAsElement} from './simple-template';
import {localize} from './amp-story-localization-service';
import objstr from 'obj-str';

/** @const {string} */
const TAG = 'amp-story-consent';
Expand All @@ -43,127 +43,80 @@ const DEFAULT_OPTIONAL_PARAMETERS = {
onlyAccept: false,
};

// TODO(gmajoulet): switch to `htmlFor` static template helper.
/**
* Story consent template.
* @param {!Element} element
* @param {!Object} config
* @param {string} consentId
* @param {?string} logoSrc
* @return {!./simple-template.ElementDef}
* @return {!Element}
* @private @const
*/
const getTemplate = (element, config, consentId, logoSrc) => ({
tag: 'div',
attrs: dict({
'class': 'i-amphtml-story-consent i-amphtml-story-system-reset',
}),
children: [
{
tag: 'div',
attrs: dict({'class': 'i-amphtml-story-consent-overflow'}),
children: [
{
tag: 'div',
attrs: dict({'class': 'i-amphtml-story-consent-container'}),
children: [
{
tag: 'div',
attrs: dict({'class': 'i-amphtml-story-consent-header'}),
children: [
{
tag: 'div',
attrs: dict({
'class': 'i-amphtml-story-consent-logo',
'style': logoSrc
? `background-image: url('${logoSrc}') !important;`
: '',
}),
},
],
},
{
tag: 'div',
attrs: dict({'class': 'i-amphtml-story-consent-content'}),
children: [
{
tag: 'h3',
children: [config.title],
},
{
tag: 'p',
children: [config.message],
},
{
tag: 'ul',
attrs: dict({'class': 'i-amphtml-story-consent-vendors'}),
children:
config.vendors &&
config.vendors.map((vendor) => ({
tag: 'li',
attrs: dict({'class': 'i-amphtml-story-consent-vendor'}),
children: [vendor],
})),
},
{
tag: 'a',
attrs: dict({
'class':
'i-amphtml-story-consent-external-link ' +
(!(config.externalLink.title && config.externalLink.href)
? 'i-amphtml-hidden'
: ''),
'href': config.externalLink.href,
'target': '_top',
'title': config.externalLink.title,
}),
children: [config.externalLink.title],
},
],
},
],
},
{
tag: 'div',
attrs: dict({'class': 'i-amphtml-story-consent-actions'}),
children: [
{
tag: 'button',
attrs: dict({
'class':
'i-amphtml-story-consent-action ' +
'i-amphtml-story-consent-action-reject' +
(config.onlyAccept === true ? ' i-amphtml-hidden' : ''),
'on': `tap:${consentId}.reject`,
}),
children: [
localize(
element,
LocalizedStringId.AMP_STORY_CONSENT_DECLINE_BUTTON_LABEL
),
],
},
{
tag: 'button',
attrs: dict({
'class':
'i-amphtml-story-consent-action ' +
'i-amphtml-story-consent-action-accept',
'on': `tap:${consentId}.accept`,
}),
children: [
localize(
element,
LocalizedStringId.AMP_STORY_CONSENT_ACCEPT_BUTTON_LABEL
),
],
},
],
},
],
},
],
});
const renderElement = (element, config, consentId, logoSrc) => (
<div class="i-amphtml-story-consent i-amphtml-story-system-reset">
<div class="i-amphtml-story-consent-overflow">
<div class="i-amphtml-story-consent-container">
<div class="i-amphtml-story-consent-header">
<div
class="i-amphtml-story-consent-logo"
style={
logoSrc ? `background-image: url('${logoSrc}') !important;` : ''
}
/>
</div>
<div class="i-amphtml-story-consent-content">
<h3>{config.title}</h3>
<p>{config.message}</p>
<ul class="i-amphtml-story-consent-vendors">
{config.vendors?.map((vendor) => (
<li class="i-amphtml-story-consent-vendor">{vendor}</li>
))}
</ul>
<a
class={objstr({
'i-amphtml-story-consent-external-link': true,
'i-amphtml-hidden': !(
config.externalLink.title && config.externalLink.href
),
})}
href={config.externalLink.href}
target="_top"
title={config.externalLink.title}
>
{config.externalLink.title}
</a>
</div>
</div>
<div class="i-amphtml-story-consent-actions">
<button
class={objstr({
'i-amphtml-story-consent-action': true,
'i-amphtml-story-consent-action-reject': true,
'i-amphtml-hidden': config.onlyAccept === true,
})}
on={`tap:${consentId}.reject`}
>
{localize(
element,
LocalizedStringId.AMP_STORY_CONSENT_DECLINE_BUTTON_LABEL
)}
</button>
<button
class={objstr({
'i-amphtml-story-consent-action': true,
'i-amphtml-story-consent-action-accept': true,
})}
on={`tap:${consentId}.accept`}
>
{localize(
element,
LocalizedStringId.AMP_STORY_CONSENT_ACCEPT_BUTTON_LABEL
)}
</button>
</div>
</div>
</div>
);

/**
* The <amp-story-consent> custom element.
Expand Down Expand Up @@ -219,9 +172,11 @@ export class AmpStoryConsent extends AMP.BaseElement {

// Story consent config is set by the `assertAndParseConfig_` method.
if (this.storyConsentConfig_) {
this.storyConsentEl_ = renderAsElement(
this.win.document,
getTemplate(this.element, this.storyConsentConfig_, consentId, logoSrc)
this.storyConsentEl_ = renderElement(
this.element,
this.storyConsentConfig_,
consentId,
logoSrc
);
createShadowRootWithStyle(this.element, this.storyConsentEl_, CSS);

Expand Down
9 changes: 4 additions & 5 deletions extensions/amp-story/1.0/amp-story-cta-layer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
*/

import {AmpStoryBaseLayer} from './amp-story-base-layer';
import {addAttributesToElement, removeElement} from '#core/dom';
import {dict} from '#core/types/object';
import {removeElement} from '#core/dom';
import {matches} from '#core/dom/query';
import {user} from '#utils/log';

Expand Down Expand Up @@ -48,17 +47,17 @@ export class AmpStoryCtaLayer extends AmpStoryBaseLayer {
setOrOverwriteAttributes_() {
const ctaLinks = this.element.querySelectorAll('a');
for (let i = 0; i < ctaLinks.length; i++) {
addAttributesToElement(ctaLinks[i], dict({'target': '_blank'}));
ctaLinks[i].setAttribute('target', '_blank');

if (!ctaLinks[i].getAttribute('role')) {
addAttributesToElement(ctaLinks[i], dict({'role': 'link'}));
ctaLinks[i].setAttribute('role', 'link');
}
}

const ctaButtons = this.element.querySelectorAll('button');
for (let i = 0; i < ctaButtons.length; i++) {
if (!ctaButtons[i].getAttribute('role')) {
addAttributesToElement(ctaButtons[i], dict({'role': 'button'}));
ctaButtons[i].setAttribute('role', 'button');
}
}
}
Expand Down

0 comments on commit b8123ef

Please sign in to comment.