Skip to content

🚀 [Story performance] Move open page attachment UI to extension #37278

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

Merged
mszylkowski merged 39 commits into from
Jan 12, 2022
Merged

🚀 [Story performance] Move open page attachment UI to extension #37278

Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
2689d02
Started moving page attachment
mszylkowski Dec 21, 2021
32e6bd5
Create bundle
mszylkowski Dec 21, 2021
778f895
Fixed tests
mszylkowski Dec 21, 2021
fd82574
Fixed import
mszylkowski Dec 21, 2021
6c023bb
Not use service getters to avoid bundling
mszylkowski Dec 21, 2021
1b372e9
Fix services
mszylkowski Dec 21, 2021
fbe589c
Fix shopping unit test
mszylkowski Dec 22, 2021
b2323ff
Removed dependency on localizationservice
mszylkowski Dec 22, 2021
6458e97
Fixed localization
mszylkowski Dec 22, 2021
68303bc
Fixed tests
mszylkowski Dec 22, 2021
d40ea05
Merge branch 'main' of github.com:ampproject/amphtml into removeAttac…
mszylkowski Dec 22, 2021
bc5942e
Sync service fetchers
mszylkowski Dec 22, 2021
fe949a0
Made services sync
mszylkowski Dec 23, 2021
65fafaa
Fixed tests for sync services
mszylkowski Dec 23, 2021
1cabd29
Remove unused import in test
mszylkowski Dec 23, 2021
a766496
AnalyticsService to sync
mszylkowski Dec 23, 2021
204c4ee
Import devAssert
mszylkowski Dec 23, 2021
d4cc178
Remove param to buildInline
mszylkowski Dec 23, 2021
35e1aab
Undo describes.js
mszylkowski Dec 23, 2021
f9cb0d7
Fix tests
mszylkowski Dec 23, 2021
bcbac30
Remove unused import
mszylkowski Dec 23, 2021
b8f903e
Debundle store with forms
mszylkowski Dec 23, 2021
7b39379
Merge branch 'main' of github.com:ampproject/amphtml into removeAttac…
mszylkowski Dec 23, 2021
e429bf4
Moved open-page-attachment
mszylkowski Dec 23, 2021
897e178
Merge branch 'main' of github.com:ampproject/amphtml into tryRemoveAt…
mszylkowski Dec 28, 2021
57b6b23
Hooked up active attribute
mszylkowski Dec 28, 2021
02b5ecb
Fixed page attachment parent element
mszylkowski Dec 28, 2021
29a58a7
Fix opening attachment
mszylkowski Dec 28, 2021
a745d50
Fixing tests
mszylkowski Dec 28, 2021
720b89e
Removed unused tests
mszylkowski Dec 28, 2021
07075f8
Fixed amp-story test
mszylkowski Dec 28, 2021
0272bc5
Dep-check
mszylkowski Dec 28, 2021
8b6599c
Zindex fix
mszylkowski Dec 28, 2021
48e514d
Moved scrim styles to page-attachment
mszylkowski Jan 4, 2022
3a0bd28
Fixed zindex
mszylkowski Jan 5, 2022
6b89a91
Merge branch 'main' of github.com:ampproject/amphtml into tryRemoveAt…
mszylkowski Jan 11, 2022
548a923
Merge branch 'main' of github.com:ampproject/amphtml into tryRemoveAt…
mszylkowski Jan 12, 2022
8d00119
Removed unused import
mszylkowski Jan 12, 2022
1c3dc84
Fixed duplicate imports in test
mszylkowski Jan 12, 2022
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
2 changes: 1 addition & 1 deletion build-system/test-configs/dep-check-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ exports.rules = [
'extensions/amp-story-page-attachment/0.1/amp-story-draggable-drawer.js->extensions/amp-story/1.0/utils.js',
'extensions/amp-story-page-attachment/0.1/amp-story-form.js->extensions/amp-story/1.0/amp-story-store-service.js',
'extensions/amp-story-page-attachment/0.1/amp-story-form.js->extensions/amp-story/1.0/loading-spinner.js',
'extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.js->extensions/amp-story/1.0/amp-story-open-page-attachment.js',
'extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js->extensions/amp-story/1.0/utils.js',
'extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.js->extensions/amp-story/1.0/amp-story-store-service.js',
'extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.js->extensions/amp-story/1.0/history.js',
'extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.js->extensions/amp-story/1.0/story-analytics.js',
Expand Down
6 changes: 3 additions & 3 deletions css/Z_INDEX.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@
| `.amp-story-draggable-drawer-root` | 4 | [extensions/amp-story-page-attachment/0.1/amp-story-draggable-drawer.css](/extensions/amp-story-page-attachment/0.1/amp-story-draggable-drawer.css) |
| `.i-amphtml-image-slider-bar` | 3 | [extensions/amp-image-slider/0.1/amp-image-slider.css](/extensions/amp-image-slider/0.1/amp-image-slider.css) |
| `styles` | 3 | [extensions/amp-story-interactive/0.1/amp-story-interactive-abstract.js](/extensions/amp-story-interactive/0.1/amp-story-interactive-abstract.js) |
| `.i-amphtml-story-page-open-attachment[active]` | 3 | [extensions/amp-story/1.0/amp-story-open-page-attachment.css](/extensions/amp-story/1.0/amp-story-open-page-attachment.css) |
| `.i-amphtml-story-page-attachment-active::after` | 3 | [extensions/amp-story/1.0/amp-story.css](/extensions/amp-story/1.0/amp-story.css) |
| `.i-amphtml-story-page-open-attachment[active]` | 3 | [extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css](/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css) |
| `.i-amphtml-story-page-attachment-active::after` | 3 | [extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.css](/extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.css) |
| `amp-story-cta-layer` | 3 | [extensions/amp-story/1.0/amp-story.css](/extensions/amp-story/1.0/amp-story.css) |
| `.i-amphtml-element > [overflow]` | 2 | [css/ampshared.css](/css/ampshared.css) |
| `body noscript > *` | 2 | [css/ampshared.css](/css/ampshared.css) |
Expand Down Expand Up @@ -162,6 +162,6 @@
| `.i-amphtml-carousel-spacer` | -1 | [extensions/amp-base-carousel/0.1/carousel.css](/extensions/amp-base-carousel/0.1/carousel.css) |
| `.i-amphtml-mega-menu-mask` | -1 | [extensions/amp-mega-menu/0.1/amp-mega-menu.css](/extensions/amp-mega-menu/0.1/amp-mega-menu.css) |
| `.i-amphtml-story-interactive-slider-bubble::after` | -1 | [extensions/amp-story-interactive/0.1/amp-story-interactive-slider.css](/extensions/amp-story-interactive/0.1/amp-story-interactive-slider.css) |
| `.i-amphtml-story-page-open-attachment-icon::after` | -1 | [extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css](/extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.css) |
| `.i-amphtml-story-access-logo::before` | -1 | [extensions/amp-story/1.0/amp-story-access.css](/extensions/amp-story/1.0/amp-story-access.css) |
| `.i-amphtml-story-consent-logo::before` | -1 | [extensions/amp-story/1.0/amp-story-consent.css](/extensions/amp-story/1.0/amp-story-consent.css) |
| `.i-amphtml-story-page-open-attachment-icon::after` | -1 | [extensions/amp-story/1.0/amp-story-open-page-attachment.css](/extensions/amp-story/1.0/amp-story-open-page-attachment.css) |
0 ...ry/1.0/amp-story-open-page-attachment.css → ...nt/0.1/amp-story-open-page-attachment.css
View file
Open in desktop
File renamed without changes.
37 changes: 23 additions & 14 deletions ...ory/1.0/amp-story-open-page-attachment.js → ...ent/0.1/amp-story-open-page-attachment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
import * as Preact from '#core/dom/jsx';
/**
* @fileoverview Helper for amp-story rendering of page-attachment UI.
*/
import {LocalizedStringId_Enum} from '#service/localization/strings';
import * as Preact from '#core/dom/jsx';
import {scopedQuerySelector} from '#core/dom/query';
import {computedStyle, setImportantStyles} from '#core/dom/style';
import {getWin} from '#core/window';

import {Services} from '#service';
import {LocalizedStringId_Enum} from '#service/localization/strings';

import {dev} from '#utils/log';
import {localize} from './amp-story-localization-service';

import {
getRGBFromCssColorValue,
getTextColorForRGB,
maybeMakeProxyUrl,
} from './utils';
import {getWin} from '#core/window';
import {scopedQuerySelector} from '#core/dom/query';
} from '../../amp-story/1.0/utils';

/**
* @enum {string}
Expand Down Expand Up @@ -94,14 +97,20 @@ const ctaLabelFromAttr = (element) =>
* @param {?string} label
* @return {?string}
*/
const openLabelOrFallback = (element, attachmentEl, label) =>
attachmentEl.tagName === 'AMP-STORY-SHOPPING-ATTACHMENT'
? localize(element, LocalizedStringId_Enum.AMP_STORY_SHOPPING_CTA_LABEL)
Copy link
Copy Markdown
Contributor

@processprocess processprocess Jan 4, 2022

Choose a reason for hiding this comment

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

Why not use the localize helper?

Copy link
Copy Markdown
Contributor Author

@mszylkowski mszylkowski Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The localize helper is used inside amp-story because part of it is "instantiate a new LocalizationService if not instantiated already" (since the amp-story bundle has that service, it's free to do that in there). On extensions we don't want to add the service in the bundle, just fetch it from the document, so we can't use that localize helper or we would be bundling it into all the extensions that need it.

: label?.trim() ||
localize(
element,
LocalizedStringId_Enum.AMP_STORY_PAGE_ATTACHMENT_OPEN_LABEL
);
const openLabelOrFallback = (element, attachmentEl, label) => {
const localizationService = Services.localizationForDoc(element);
if (attachmentEl.tagName === 'AMP-STORY-SHOPPING-ATTACHMENT') {
return localizationService.getLocalizedString(
LocalizedStringId_Enum.AMP_STORY_SHOPPING_CTA_LABEL
);
}
return (
label?.trim() ||
localizationService.getLocalizedString(
LocalizedStringId_Enum.AMP_STORY_PAGE_ATTACHMENT_OPEN_LABEL
)
);
};

/**
* Renders inline page attachment UI.
Expand Down
24 changes: 24 additions & 0 deletions extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,27 @@ amp-story[desktop] .i-amphtml-story-page-attachment-remote {
opacity: 1;
}
}

/** Scrim on amp-story-page when attachment is active */
.i-amphtml-story-page-attachment-active::after {
content: '' !important;
display: block !important;
left: 0 !important;
top: 0 !important;
bottom: 0 !important;
right: 0 !important;
position: absolute !important;
background: #000 !important;
z-index: 3 !important;
animation: i-amphtml-attachment-overlay-opacity 0.3s cubic-bezier(0.0, 0.0, 0.2, 1) forwards !important;
}

@keyframes i-amphtml-attachment-overlay-opacity {
0% {
opacity: 0;
}

100% {
opacity: 0.5;
}
}
96 changes: 90 additions & 6 deletions extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {devAssert} from '#core/assert';
import {removeElement} from '#core/dom';
import * as Preact from '#core/dom/jsx';
import {closest} from '#core/dom/query';
import {closest, closestAncestorElementBySelector} from '#core/dom/query';
import {toggle} from '#core/dom/style';
import {getHistoryState} from '#core/window/history';

Expand All @@ -14,21 +15,26 @@ import {
allowlistFormActions,
setupResponseAttributeElements,
} from './amp-story-form';

import {CSS} from '../../../build/amp-story-page-attachment-0.1.css';
import {getSourceOrigin} from '../../../src/url';
import {
AttachmentTheme,
renderOutlinkLinkIconElement,
} from '../../amp-story/1.0/amp-story-open-page-attachment';
renderPageAttachmentUI,
} from './amp-story-open-page-attachment';

import {CSS as pageAttachmentCss} from '../../../build/amp-story-open-page-attachment-0.1.css';
import {CSS} from '../../../build/amp-story-page-attachment-0.1.css';
import {getSourceOrigin} from '../../../src/url';
import {
Action,
StateProperty,
UIType,
} from '../../amp-story/1.0/amp-story-store-service';
import {HistoryState, setHistoryState} from '../../amp-story/1.0/history';
import {StoryAnalyticsEvent} from '../../amp-story/1.0/story-analytics';
import {triggerClickFromLightDom} from '../../amp-story/1.0/utils';
import {
createShadowRootWithStyle,
triggerClickFromLightDom,
} from '../../amp-story/1.0/utils';

/**
* Distance to swipe before opening attachment.
Expand Down Expand Up @@ -65,6 +71,16 @@ export class AmpStoryPageAttachment extends DraggableDrawer {
constructor(element) {
super(element);

/** @private @const {!Element} */
this.storyEl_ = devAssert(
closestAncestorElementBySelector(this.element, 'amp-story')
);

/** @private @const {!Element} */
this.pageEl_ = devAssert(
closestAncestorElementBySelector(this.element, 'amp-story-page')
);

/** @private @const {!../../amp-story/1.0/story-analytics.StoryAnalyticsService} */
this.analyticsService_ = Services.storyAnalyticsService(this.win);

Expand All @@ -80,6 +96,7 @@ export class AmpStoryPageAttachment extends DraggableDrawer {
*/
buildCallback() {
super.buildCallback();
this.buildOpenAttachmentUI_();
this.maybeSetDarkThemeForElement_(this.headerEl);
this.maybeSetDarkThemeForElement_(this.element);

Expand Down Expand Up @@ -118,6 +135,53 @@ export class AmpStoryPageAttachment extends DraggableDrawer {
}
}

/** @private */
buildOpenAttachmentUI_() {
// To prevent 'title' attribute from being used by browser, copy value to 'data-title' and remove.
if (this.element.hasAttribute('title')) {
this.element.setAttribute(
'data-title',
this.element.getAttribute('title')
);
this.element.removeAttribute('title');
}

if (!this.openAttachmentEl_) {
this.openAttachmentEl_ = renderPageAttachmentUI(
this.storyEl_,
this.element
);

// This ensures `active` is set on first render.
// Otherwise setState may be called before this.openAttachmentEl_ exists.
if (this.storyEl_.hasAttribute('active')) {
this.openAttachmentEl_.setAttribute('active', '');
}

const container = (
<div
class="i-amphtml-story-page-open-attachment-host"
role="button"
onClick={(e) => {
// Prevent default so link can be opened programmatically after URL preview is shown.
e.preventDefault();
this.open();
}}
></div>
);

this.mutateElement(() => {
this.pageEl_.appendChild(
createShadowRootWithStyle(
container,
this.openAttachmentEl_,
pageAttachmentCss
)
);
});
}
}

/**
* Builds inline page attachment's drawer UI.
* @private
Expand Down Expand Up @@ -270,6 +334,18 @@ export class AmpStoryPageAttachment extends DraggableDrawer {
this.contentEl.appendChild(link);
}

/**
* Activate or deactivate open page attachment
* @param {boolean} isActive
*/
setOpenAttachmentActive(isActive) {
if (isActive) {
this.openAttachmentEl_.setAttribute('active', '');
} else {
this.openAttachmentEl_.removeAttribute('äctive');
}
}

/**
* @override
*/
Expand Down Expand Up @@ -321,6 +397,14 @@ export class AmpStoryPageAttachment extends DraggableDrawer {
}
});
}

this.storeService.subscribe(
StateProperty.CURRENT_PAGE_ID,
(id) => {
this.setOpenAttachmentActive(id === this.pageEl_.id);
},
true
);
}

/**
Expand Down
Loading