Skip to content

Commit

Permalink
🚀♻️ Normalize how locale strings are rendered in amp-story (#36677)
Browse files Browse the repository at this point in the history
All callsites now use `localize(element, key)`. This attempts zero changes in functionality.

This function compresses better than the alternatives, which we're able remove as well:

- `getLocalizationService(...).getLocalizedString(...)`. Some callsites check for existence of the service even though it's always present, and sometimes they'd store it in a private. `localize()` is briefer.

- On `simple-template`, we remove awkward fields: `unlocalizedString` and `localizedStringId` use `children`, and `localizedLabelId` is moved to an `aria-label` in `attrs`. This allows us to simplify the `simple-template` implementation, and reformats templates so that they'll be more easily converted to JSX in the future.

Together, these changes reduce the module build by 0.15K-0.30K
  • Loading branch information
alanorozco committed Nov 1, 2021
1 parent a4dcc00 commit 50e6ed2
Show file tree
Hide file tree
Showing 18 changed files with 312 additions and 270 deletions.
39 changes: 20 additions & 19 deletions extensions/amp-story/1.0/amp-story-consent.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {isJsonScriptTag} from '#core/dom';

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

/** @const {string} */
const TAG = 'amp-story-consent';
Expand All @@ -45,13 +46,14 @@ const DEFAULT_OPTIONAL_PARAMETERS = {
// 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}
* @private @const
*/
const getTemplate = (config, consentId, logoSrc) => ({
const getTemplate = (element, config, consentId, logoSrc) => ({
tag: 'div',
attrs: dict({
'class': 'i-amphtml-story-consent i-amphtml-story-system-reset',
Expand All @@ -77,7 +79,6 @@ const getTemplate = (config, consentId, logoSrc) => ({
? `background-image: url('${logoSrc}') !important;`
: '',
}),
children: [],
},
],
},
Expand All @@ -87,15 +88,11 @@ const getTemplate = (config, consentId, logoSrc) => ({
children: [
{
tag: 'h3',
attrs: dict({}),
children: [],
unlocalizedString: config.title,
children: [config.title],
},
{
tag: 'p',
attrs: dict({}),
children: [],
unlocalizedString: config.message,
children: [config.message],
},
{
tag: 'ul',
Expand All @@ -105,8 +102,7 @@ const getTemplate = (config, consentId, logoSrc) => ({
config.vendors.map((vendor) => ({
tag: 'li',
attrs: dict({'class': 'i-amphtml-story-consent-vendor'}),
children: [],
unlocalizedString: vendor,
children: [vendor],
})),
},
{
Expand All @@ -121,8 +117,7 @@ const getTemplate = (config, consentId, logoSrc) => ({
'target': '_top',
'title': config.externalLink.title,
}),
children: [],
unlocalizedString: config.externalLink.title,
children: [config.externalLink.title],
},
],
},
Expand All @@ -141,9 +136,12 @@ const getTemplate = (config, consentId, logoSrc) => ({
(config.onlyAccept === true ? ' i-amphtml-hidden' : ''),
'on': `tap:${consentId}.reject`,
}),
children: [],
localizedStringId:
LocalizedStringId.AMP_STORY_CONSENT_DECLINE_BUTTON_LABEL,
children: [
localize(
element,
LocalizedStringId.AMP_STORY_CONSENT_DECLINE_BUTTON_LABEL
),
],
},
{
tag: 'button',
Expand All @@ -153,9 +151,12 @@ const getTemplate = (config, consentId, logoSrc) => ({
'i-amphtml-story-consent-action-accept',
'on': `tap:${consentId}.accept`,
}),
children: [],
localizedStringId:
LocalizedStringId.AMP_STORY_CONSENT_ACCEPT_BUTTON_LABEL,
children: [
localize(
element,
LocalizedStringId.AMP_STORY_CONSENT_ACCEPT_BUTTON_LABEL
),
],
},
],
},
Expand Down Expand Up @@ -220,7 +221,7 @@ export class AmpStoryConsent extends AMP.BaseElement {
if (this.storyConsentConfig_) {
this.storyConsentEl_ = renderAsElement(
this.win.document,
getTemplate(this.storyConsentConfig_, consentId, logoSrc)
getTemplate(this.element, this.storyConsentConfig_, consentId, logoSrc)
);
createShadowRootWithStyle(this.element, this.storyConsentEl_, CSS);

Expand Down
16 changes: 7 additions & 9 deletions extensions/amp-story/1.0/amp-story-draggable-drawer.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import {LocalizedStringId} from '#service/localization/strings';
import {Services} from '#service';
import {closest} from '#core/dom/query';
import {createShadowRootWithStyle} from './utils';
import {dev, devAssert} from '#utils/log';
import {getLocalizationService} from './amp-story-localization-service';
import {dev} from '#utils/log';
import {localize} from './amp-story-localization-service';
import {htmlFor} from '#core/dom/static-template';
import {isAmpElement} from '#core/dom/amp-element-helpers';
import {listen} from '#utils/event-helper';
Expand Down Expand Up @@ -142,13 +142,11 @@ export class DraggableDrawer extends AMP.BaseElement {
spacerEl.classList.add('i-amphtml-story-draggable-drawer-spacer');
spacerEl.classList.add('i-amphtml-story-system-reset');
spacerEl.setAttribute('role', 'button');
const localizationService = getLocalizationService(devAssert(this.element));
if (localizationService) {
const localizedCloseString = localizationService.getLocalizedString(
LocalizedStringId.AMP_STORY_CLOSE_BUTTON_LABEL
);
spacerEl.setAttribute('aria-label', localizedCloseString);
}
const localizedCloseString = localize(
this.element,
LocalizedStringId.AMP_STORY_CLOSE_BUTTON_LABEL
);
spacerEl.setAttribute('aria-label', localizedCloseString);
this.containerEl.insertBefore(spacerEl, this.contentEl);
this.contentEl.appendChild(headerShadowRootEl);

Expand Down
17 changes: 5 additions & 12 deletions extensions/amp-story/1.0/amp-story-embedded-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
import {dev, devAssert, user, userAssert} from '#utils/log';
import {dict} from '#core/types/object';
import {getAmpdoc} from '../../../src/service-helpers';
import {getLocalizationService} from './amp-story-localization-service';
import {localize} from './amp-story-localization-service';
import {htmlFor, htmlRefs} from '#core/dom/static-template';
import {isProtocolValid, parseUrlDeprecated} from '../../../src/url';

Expand Down Expand Up @@ -662,15 +662,10 @@ export class AmpStoryEmbeddedComponent {
'.i-amphtml-expanded-view-close-button'
)
);
const localizationService = getLocalizationService(
devAssert(this.storyEl_)
closeButton.setAttribute(
'aria-label',
localize(this.storyEl_, LocalizedStringId.AMP_STORY_CLOSE_BUTTON_LABEL)
);
if (localizationService) {
const localizedCloseString = localizationService.getLocalizedString(
LocalizedStringId.AMP_STORY_CLOSE_BUTTON_LABEL
);
closeButton.setAttribute('aria-label', localizedCloseString);
}
this.mutator_.mutateElement(dev().assertElement(this.componentPage_), () =>
this.componentPage_.appendChild(this.expandedViewOverlay_)
);
Expand Down Expand Up @@ -1131,9 +1126,7 @@ export class AmpStoryEmbeddedComponent {
updateTooltipText_(target, embedConfig) {
const tooltipText =
target.getAttribute('data-tooltip-text') ||
getLocalizationService(this.storyEl_).getLocalizedString(
embedConfig.localizedStringId
) ||
localize(this.storyEl_, embedConfig.localizedStringId) ||
getSourceOriginForElement(target, this.getElementHref_(target));
const existingTooltipText = this.tooltip_.querySelector(
'.i-amphtml-tooltip-text'
Expand Down
7 changes: 3 additions & 4 deletions extensions/amp-story/1.0/amp-story-form.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import {Action, getStoreService} from './amp-story-store-service';
import {LoadingSpinner} from './loading-spinner';
import {LocalizedStringId} from '#service/localization/strings';
import {devAssert} from '#utils/log';
import {escapeCssSelectorIdent} from '#core/dom/css-selectors';
import {getLocalizationService} from './amp-story-localization-service';
import {localize} from './amp-story-localization-service';
import {htmlFor} from '#core/dom/static-template';
import {scopedQuerySelector, scopedQuerySelectorAll} from '#core/dom/query';

Expand Down Expand Up @@ -123,8 +122,8 @@ function createFormResultEl_(win, formEl, isSuccess) {
resultEl.firstElementChild.appendChild(iconEl);

const textEl = win.document.createElement('div');
const localizationService = getLocalizationService(devAssert(win.document));
textEl.textContent = localizationService.getLocalizedString(
textEl.textContent = localize(
win.document,
isSuccess
? LocalizedStringId.AMP_STORY_FORM_SUBMIT_SUCCESS
: LocalizedStringId.AMP_STORY_FORM_SUBMIT_ERROR
Expand Down
31 changes: 23 additions & 8 deletions extensions/amp-story/1.0/amp-story-hint.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,13 @@ import {Services} from '#service';
import {createShadowRootWithStyle} from './utils';
import {dict} from '#core/types/object';
import {renderAsElement} from './simple-template';
import {localize} from './amp-story-localization-service';

/** @private @const {!./simple-template.ElementDef} */
const TEMPLATE = {
/**
* @param {!Element} element
* @return {!./simple-template.ElementDef}
*/
const getTemplate = (element) => ({
tag: 'aside',
attrs: dict({
'class':
Expand Down Expand Up @@ -51,8 +55,12 @@ const TEMPLATE = {
attrs: dict({
'class': 'i-amphtml-story-hint-tap-button-text',
}),
localizedStringId:
LocalizedStringId.AMP_STORY_HINT_UI_PREVIOUS_LABEL,
children: [
localize(
element,
LocalizedStringId.AMP_STORY_HINT_UI_PREVIOUS_LABEL
),
],
},
],
},
Expand Down Expand Up @@ -85,8 +93,12 @@ const TEMPLATE = {
attrs: dict({
'class': 'i-amphtml-story-hint-tap-button-text',
}),
localizedStringId:
LocalizedStringId.AMP_STORY_HINT_UI_NEXT_LABEL,
children: [
localize(
element,
LocalizedStringId.AMP_STORY_HINT_UI_NEXT_LABEL
),
],
},
],
},
Expand All @@ -95,7 +107,7 @@ const TEMPLATE = {
],
},
],
};
});

/** @type {string} */
const NAVIGATION_OVERLAY_CLASS = 'show-navigation-overlay';
Expand Down Expand Up @@ -157,7 +169,10 @@ export class AmpStoryHint {
this.isBuilt_ = true;

const root = this.document_.createElement('div');
this.hintContainer_ = renderAsElement(this.document_, TEMPLATE);
this.hintContainer_ = renderAsElement(
this.document_,
getTemplate(this.parentEl_)
);
createShadowRootWithStyle(root, this.hintContainer_, CSS);

this.storeService_.subscribe(
Expand Down
11 changes: 5 additions & 6 deletions extensions/amp-story/1.0/amp-story-info-dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {closest, matches} from '#core/dom/query';
import {createShadowRootWithStyle, triggerClickFromLightDom} from './utils';
import {dev} from '#utils/log';
import {getAmpdoc} from '../../../src/service-helpers';
import {getLocalizationService} from './amp-story-localization-service';
import {localize} from './amp-story-localization-service';
import {htmlFor} from '#core/dom/static-template';

/** @const {string} Class to toggle the info dialog. */
Expand Down Expand Up @@ -48,9 +48,6 @@ export class InfoDialog {
/** @private {boolean} */
this.isBuilt_ = false;

/** @private @const {!../../../src/service/localization.LocalizationService} */
this.localizationService_ = getLocalizationService(parentEl);

/** @private @const {!./amp-story-store-service.AmpStoryStoreService} */
this.storeService_ = getStoreService(this.win_);

Expand Down Expand Up @@ -206,7 +203,8 @@ export class InfoDialog {
* @return {*} TODO(#23582): Specify return type
*/
setHeading_() {
const label = this.localizationService_.getLocalizedString(
const label = localize(
this.parentEl_,
LocalizedStringId.AMP_STORY_DOMAIN_DIALOG_HEADING_LABEL
);
const headingEl = dev().assertElement(
Expand Down Expand Up @@ -252,7 +250,8 @@ export class InfoDialog {
);

return this.mutator_.mutateElement(this.moreInfoLinkEl_, () => {
const label = this.localizationService_.getLocalizedString(
const label = localize(
this.parentEl_,
LocalizedStringId.AMP_STORY_DOMAIN_DIALOG_HEADING_LINK
);
this.moreInfoLinkEl_.classList.add(MOREINFO_VISIBLE_CLASS);
Expand Down
15 changes: 13 additions & 2 deletions extensions/amp-story/1.0/amp-story-localization-service.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import {LocalizationService} from '#service/localization';
import {Services} from '#service';
import {registerServiceBuilderForDoc} from '../../../src/service-helpers';
// eslint-disable-next-line no-unused-vars
import {LocalizedStringId} from '#service/localization/strings';

/**
* Util function to retrieve the localization service. Ensures we can retrieve
Expand All @@ -9,7 +11,7 @@ import {registerServiceBuilderForDoc} from '../../../src/service-helpers';
* @param {!Element} element
* @return {!../../../src/service/localization.LocalizationService}
*/
export const getLocalizationService = (element) => {
export function getLocalizationService(element) {
let localizationService = Services.localizationForDoc(element);

if (!localizationService) {
Expand All @@ -20,4 +22,13 @@ export const getLocalizationService = (element) => {
}

return localizationService;
};
}

/**
* @param {!Node} context
* @param {LocalizedStringId} key
* @return {?string}
*/
export function localize(context, key) {
return getLocalizationService(context).getLocalizedString(key);
}
10 changes: 3 additions & 7 deletions extensions/amp-story/1.0/amp-story-open-page-attachment.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {AttachmentTheme} from './amp-story-page-attachment';
import {LocalizedStringId} from '#service/localization/strings';
import {computedStyle, setImportantStyles} from '#core/dom/style';
import {dev} from '#utils/log';
import {getLocalizationService} from './amp-story-localization-service';
import {localize} from './amp-story-localization-service';
import {
getRGBFromCssColorValue,
getTextColorForRGB,
Expand Down Expand Up @@ -133,9 +133,7 @@ const renderOutlinkUI = (pageEl, attachmentEl) => {
attachmentEl.getAttribute('data-cta-text');
const openLabel = openLabelAttr
? openLabelAttr.trim()
: getLocalizationService(pageEl).getLocalizedString(
LocalizedStringId.AMP_STORY_PAGE_ATTACHMENT_OPEN_LABEL
);
: localize(pageEl, LocalizedStringId.AMP_STORY_PAGE_ATTACHMENT_OPEN_LABEL);
ctaLabelEl.textContent = openLabel;
openAttachmentEl.setAttribute('aria-label', openLabel);

Expand Down Expand Up @@ -178,9 +176,7 @@ const renderInlineUi = (pageEl, attachmentEl) => {
attachmentEl.getAttribute('data-cta-text');
const openLabel =
(openLabelAttr && openLabelAttr.trim()) ||
getLocalizationService(pageEl).getLocalizedString(
LocalizedStringId.AMP_STORY_PAGE_ATTACHMENT_OPEN_LABEL
);
localize(pageEl, LocalizedStringId.AMP_STORY_PAGE_ATTACHMENT_OPEN_LABEL);
openAttachmentEl.setAttribute('aria-label', openLabel);

if (openLabel !== 'none') {
Expand Down

0 comments on commit 50e6ed2

Please sign in to comment.