Skip to content

🐛 amp-story-shopping does not display CTA if all tags are invalid and Fixes remote shopping data loading on refresh with page attachment open #38035

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
jshamble merged 28 commits into from
Apr 19, 2022
Merged

🐛 amp-story-shopping does not display CTA if all tags are invalid and Fixes remote shopping data loading on refresh with page attachment open #38035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
c32eeaa
added fixed for shopping tag rendering
jshamble Apr 7, 2022
9c807f0
added shopping json patch that fixed regression
jshamble Apr 11, 2022
86dd0f3
resolved promise .all
jshamble Apr 11, 2022
ab39a72
updated template
jshamble Apr 11, 2022
0bb58ce
refactored state update
jshamble Apr 11, 2022
52c0312
fixed promise resolution
jshamble Apr 12, 2022
2088558
added more descriptive error message
jshamble Apr 15, 2022
5618cb7
cahnged order of ternary
jshamble Apr 15, 2022
05cac5f
added history check for page refresh
jshamble Apr 18, 2022
d341d6b
updated hsitory service to store all data, not jsut the activeData
jshamble Apr 18, 2022
781fe65
added shopping element
jshamble Apr 18, 2022
2103d85
added shopping attachment logic for page refresh, added back active p…
jshamble Apr 18, 2022
0b685ac
added set history state test
jshamble Apr 18, 2022
8d05b46
cleaned up some minor nits
jshamble Apr 18, 2022
a2e94fe
added unit test for shopping tag history
jshamble Apr 18, 2022
f818401
history stub
jshamble Apr 18, 2022
d0f5d36
added shopping if clause optimizations
jshamble Apr 19, 2022
a87a927
added dependencies
jshamble Apr 19, 2022
cfb87f2
added dependencies
jshamble Apr 19, 2022
0b78d51
added dependencies
jshamble Apr 19, 2022
8a3fcd3
updated unit tests
jshamble Apr 19, 2022
723981b
updated unit tests
jshamble Apr 19, 2022
e05b6ce
updated unit tests
jshamble Apr 19, 2022
ed8d8f3
removed check
jshamble Apr 19, 2022
b75b6e5
browser test safari
jshamble Apr 19, 2022
b44c364
browser test safari
jshamble Apr 19, 2022
b5130a5
added better url checks
jshamble Apr 19, 2022
66e7b9a
added stuff
jshamble Apr 19, 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: 2 additions & 0 deletions 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 @@ -248,8 +248,10 @@ exports.rules = [
'extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js->extensions/amp-story/1.0/amp-story-store-service.js',
'extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js->extensions/amp-story/1.0/variable-service.js',
'extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js->extensions/amp-story/1.0/story-analytics.js',
'extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js->extensions/amp-story/1.0/history.js',
'extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js->extensions/amp-story/1.0/variable-service.js',
'extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js->extensions/amp-story/1.0/story-analytics.js',
'extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js->extensions/amp-story/1.0/history.js',

// Interactive components that depend on story functionality.
'extensions/amp-story-interactive/0.1/amp-story-interactive-abstract.js->extensions/amp-story/1.0/amp-story-store-service.js',
Expand Down
30 changes: 19 additions & 11 deletions extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {Services} from '#service';
import {LocalizedStringId_Enum} from '#service/localization/strings';

import {localizeTemplate} from 'extensions/amp-story/1.0/amp-story-localization-service';
import {HistoryState, setHistoryState} from 'extensions/amp-story/1.0/history';

import {formatI18nNumber, loadFonts} from './amp-story-shopping';
import {
Expand Down Expand Up @@ -82,17 +83,25 @@ export class AmpStoryShoppingAttachment extends AMP.BaseElement {
this.pageEl_.querySelectorAll('amp-story-shopping-tag')
);

getShoppingConfig(this.element, this.pageEl_.id).then((config) =>
storeShoppingConfig(this.pageEl_, config)
Comment thread
jshamble marked this conversation as resolved.
);

if (this.shoppingTags_.length === 0) {
return;
return Promise.reject(
new Error(`No shopping tags on page ${this.pageEl_.id}.`)
);
}

return this.localizationService_
.getLocalizedStringAsync(
LocalizedStringId_Enum.AMP_STORY_SHOPPING_CTA_LABEL
return getShoppingConfig(this.element, this.pageEl_.id)
.then((config) => {
if (Object.keys(config).length === 0) {
return Promise.reject(
new Error(`No valid shopping data on page ${this.pageEl_.id}.`)
);
}
storeShoppingConfig(this.pageEl_, config);
})
.then(() =>
this.localizationService_.getLocalizedStringAsync(
LocalizedStringId_Enum.AMP_STORY_SHOPPING_CTA_LABEL
)
)
.then((ctaText) => {
this.attachmentEl_ = (
Expand All @@ -110,9 +119,6 @@ export class AmpStoryShoppingAttachment extends AMP.BaseElement {

/** @override */
layoutCallback() {
if (this.shoppingTags_.length === 0) {
Comment thread
jshamble marked this conversation as resolved.
return;
}
loadFonts(this.win, FONTS_TO_LOAD);
// Update template on attachment state update or shopping data update.
this.storeService_.subscribe(
Expand Down Expand Up @@ -299,6 +305,8 @@ export class AmpStoryShoppingAttachment extends AMP.BaseElement {
this.storeService_.dispatch(Action.ADD_SHOPPING_DATA, {
'activeProductData': shoppingData,
});

setHistoryState(this.win, HistoryState.SHOPPING_DATA, shoppingData);
}

/**
Expand Down
4 changes: 4 additions & 0 deletions extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {

import {Services} from '#service';

import {HistoryState, setHistoryState} from 'extensions/amp-story/1.0/history';

import {formatI18nNumber, loadFonts} from './amp-story-shopping';

import {CSS as shoppingSharedCSS} from '../../../build/amp-story-shopping-shared-0.1.css';
Expand Down Expand Up @@ -170,6 +172,8 @@ export class AmpStoryShoppingTag extends AMP.BaseElement {
'activeProductData': this.tagData_,
});

setHistoryState(this.win, HistoryState.SHOPPING_DATA, this.tagData_);

this.variableService_.onVariableUpdate(
AnalyticsVariable.STORY_SHOPPING_PRODUCT_ID,
this.tagData_.productId
Expand Down
19 changes: 19 additions & 0 deletions extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-attachment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ import * as Preact from '#core/dom/jsx';

import {Services} from '#service';

import {HistoryState} from 'extensions/amp-story/1.0/history';
import * as history from 'extensions/amp-story/1.0/history';

import '../../../amp-story/1.0/amp-story';
import '../../../amp-story/1.0/amp-story-page';
import '../amp-story-shopping';
Expand Down Expand Up @@ -275,5 +278,21 @@ describes.realWin(
StoryAnalyticsEvent.SHOPPING_PDP_VIEW
);
});

it('should call history service on Product Listing Page card click', async () => {
const historyStub = env.sandbox.stub(history, 'setHistoryState');

await layoutShoppingImplAndAttachmentChildImpl();
storeService.dispatch(Action.TOGGLE_PAGE_ATTACHMENT_STATE, true);
const plpCard = attachmentChildEl.querySelector(
'.i-amphtml-amp-story-shopping-plp-card'
);
plpCard.dispatchEvent(new Event('click'));
expect(historyStub).to.have.been.called.calledWith(
win,
HistoryState.SHOPPING_DATA,
shoppingData.items[0]
);
});
}
);
21 changes: 21 additions & 0 deletions extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-tag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ import '../amp-story-shopping';
import {Services} from '#service';
import {LocalizationService} from '#service/localization';

import '../../../amp-story-page-attachment/0.1/amp-story-page-attachment';

import * as history from 'extensions/amp-story/1.0/history';
import {HistoryState} from 'extensions/amp-story/1.0/history';

import {registerServiceBuilder} from '../../../../src/service-helpers';
import {
Action,
Expand Down Expand Up @@ -128,11 +133,27 @@ describes.realWin(

it('should call analytics service on tag click', async () => {
const trigger = env.sandbox.stub(analytics, 'triggerEvent');
env.sandbox.stub(history, 'setHistoryState');
await setupShoppingTagAndData();
await shoppingTag.shoppingTagEl_.click();
expect(trigger).to.have.been.calledWith(
StoryAnalyticsEvent.SHOPPING_TAG_CLICK
);
});

it('should call history service on tag click', async () => {
const tagData = {
'productTitle': 'Spectacular Spectacles',
};

const historyStub = env.sandbox.stub(history, 'setHistoryState');
await setupShoppingTagAndData();
await shoppingTag.shoppingTagEl_.click();
expect(historyStub).to.have.been.called.calledWith(
win,
HistoryState.SHOPPING_DATA,
tagData
);
});
}
);
11 changes: 11 additions & 0 deletions extensions/amp-story/1.0/amp-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,17 @@ export class AmpStory extends AMP.BaseElement {
attachmentImpl.open(false /** shouldAnimate */)
);
}

const shoppingData = getHistoryState(
this.win,
HistoryState.SHOPPING_DATA
);

if (shoppingData) {
this.storeService_.dispatch(Action.ADD_SHOPPING_DATA, {
Comment thread
jshamble marked this conversation as resolved.
'activeProductData': shoppingData,
});
}
}

if (
Expand Down
1 change: 1 addition & 0 deletions extensions/amp-story/1.0/history.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const LOCAL_STORAGE_KEY = 'amp-story-state';
export const HistoryState = {
ATTACHMENT_PAGE_ID: 'ampStoryAttachmentPageId',
NAVIGATION_PATH: 'ampStoryNavigationPath',
SHOPPING_DATA: 'ampStoryShoppingData',
};

/**
Expand Down