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

amp-list: Hide placeholder et al. after re-render with local data #17209

Merged
merged 5 commits into from
Jul 31, 2018
Merged

amp-list: Hide placeholder et al. after re-render with local data #17209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c7b67e8
Hide placeholder/fallback/loading after rerender with local data.
Jul 31, 2018
86de0d7
Add comments.
Jul 31, 2018
640e616
Skip failing test-selector-list tests.
Jul 31, 2018
86391f0
Fix test-selector-list.js tests
kevinkassimo Jul 31, 2018
06e8f6d
Merge pull request #1 from kevinkassimo/fix-test-selector-list
Jul 31, 2018
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
28 changes: 22 additions & 6 deletions examples/bind/list.amp.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,23 @@
quantity: 1976,
unitPrice: 3.50
}, {
name: 'ice cream',
name: 'burger',
quantity: 108,
unitPrice: 6.50
}]
})">Show Other Foods</button>
})">Show Junk Food (local)</button>

<button on="tap:AMP.setState({
src: [{
name: 'cake',
quantity: 424,
unitPrice: 14.2
}, {
name: 'ice cream',
quantity: 23,
unitPrice: 0.78
}]
})">Show Desserts (local)</button>

<button on="tap:AMP.setState({foo: foo+1})">increment foo</button>

Expand All @@ -40,18 +52,22 @@
</template>

<h3>Default</h3>
<amp-list layout="responsive" src="/list/fruit-data/get" [src]="src" width="600" height="100" template="my-template">
<amp-list layout="responsive" src="/list/fruit-data/get" [src]="src" width="600" height="100" template="my-template" reset-on-refresh>
<div placeholder>(Placeholder)</div>
</amp-list>

<h3>binding="always"</h3>
<amp-list binding="always" layout="responsive" src="/list/fruit-data/get" [src]="src" width="600" height="100" template="my-template">
<amp-list binding="always" layout="responsive" src="/list/fruit-data/get" [src]="src" width="600" height="100" template="my-template" reset-on-refresh>
<div placeholder>(Placeholder)</div>
</amp-list>

<h3>binding="refresh"</h3>
<amp-list binding="refresh" layout="responsive" src="/list/fruit-data/get" [src]="src" width="600" height="100" template="my-template">
<amp-list binding="refresh" layout="responsive" src="/list/fruit-data/get" [src]="src" width="600" height="100" template="my-template" reset-on-refresh>
<div placeholder>(Placeholder)</div>
</amp-list>

<h3>binding="no"</h3>
<amp-list binding="no" layout="responsive" src="/list/fruit-data/get" [src]="src" width="600" height="100" template="my-template">
<amp-list binding="no" layout="responsive" src="/list/fruit-data/get" [src]="src" width="600" height="100" template="my-template" reset-on-refresh>
<div placeholder>(Placeholder)</div>
</amp-list>
</body>
53 changes: 22 additions & 31 deletions extensions/amp-list/0.1/amp-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,22 +148,20 @@ export class AmpList extends AMP.BaseElement {
// Defer to fetch in layoutCallback() before first layout.
if (this.layoutCompleted_) {
this.resetIfNecessary_();
return this.fetchList_();
this.fetchList_();
}
} else if (typeof src === 'object') {
// Remove the 'src' now that local data is used to render the list.
this.element.setAttribute('src', '');
this.resetIfNecessary_();
const items = isArray(src) ? src : [src];
this.scheduleRender_(items);
this.scheduleRender_(isArray(src) ? src : [src]);
} else {
this.user().error(TAG, 'Unexpected "src" type: ' + src);
}
} else if (state !== undefined) {
user().error(TAG, '[state] is deprecated, please use [src] instead.');
this.resetIfNecessary_();
const items = isArray(state) ? state : [state];
this.scheduleRender_(items);
this.scheduleRender_(isArray(state) ? state : [state]);
}
}

Expand All @@ -177,26 +175,17 @@ export class AmpList extends AMP.BaseElement {
}

/**
* Wraps `toggleFallback()`. Runs in a mutate context by default but can be
* disabled by passing false to `mutate`.
* Wraps `toggleFallback()`. Must be called in a mutate context.
* @param {boolean} show
* @param {boolean=} mutate
* @private
*/
toggleFallback_(show, mutate = true) {
toggleFallback_(show) {
// Early-out if toggling would be a no-op.
if (!show && !this.fallbackDisplayed_) {
return;
}
const toggle = value => {
this.toggleFallback(value);
this.fallbackDisplayed_ = value;
};
if (mutate) {
this.mutateElement(() => toggle(show));
} else {
toggle(show);
}
this.toggleFallback(show);
this.fallbackDisplayed_ = show;
}

/**
Expand All @@ -209,7 +198,7 @@ export class AmpList extends AMP.BaseElement {
this.togglePlaceholder(true);
this.toggleLoading(true, /* opt_force */ true);
this.mutateElement(() => {
this.toggleFallback_(false, /* mutate */ false);
this.toggleFallback_(false);
removeChildren(dev().assertElement(this.container_));
});
}
Expand Down Expand Up @@ -250,7 +239,7 @@ export class AmpList extends AMP.BaseElement {
return this.scheduleRender_(items);
}, error => {
throw user().createError('Error fetching amp-list', error);
}).then(() => this.onFetchSuccess_(), error => this.onFetchError_(error));
}).catch(error => this.showFallback_(error));
}
}

Expand All @@ -264,11 +253,11 @@ export class AmpList extends AMP.BaseElement {
const data = getData(resp);
user().assert(
resp && (typeof data !== 'undefined'),
'Response missing the \'data\' field');
'Response missing the "data" field.');
return this.scheduleRender_(data);
}, error => {
throw user().createError('Error proxying amp-list templates', error);
}).then(() => this.onFetchSuccess_(), error => this.onFetchError_(error));
}).catch(error => this.showFallback_(error));
}

/**
Expand Down Expand Up @@ -382,8 +371,9 @@ export class AmpList extends AMP.BaseElement {
*/
render_(elements) {
dev().info(TAG, 'render:', elements);

this.mutateElement(() => {
this.hideFallbackAndPlaceholder_();

removeChildren(dev().assertElement(this.container_));
elements.forEach(element => {
if (!element.hasAttribute('role')) {
Expand Down Expand Up @@ -424,33 +414,34 @@ export class AmpList extends AMP.BaseElement {
return batchFetchJsonFor(ampdoc, this.element, itemsExpr, policy);
}

/** @private */
onFetchSuccess_() {
/**
* Must be called in mutate context.
* @private
*/
hideFallbackAndPlaceholder_() {
this.toggleLoading(false);
if (this.getFallback()) {
// Hide in case fallback was displayed for a previous fetch.
this.toggleFallback_(false);
}
this.togglePlaceholder(false);
this.toggleLoading(false);
}

/**
* @param {*=} error
* @throws {!Error} If fallback element is not present.
* @private
* @throws {!Error} throws error if fallback element is not present.
*/
onFetchError_(error) {
showFallback_(error) {
this.toggleLoading(false);
if (this.getFallback()) {
this.toggleFallback(true);
this.toggleFallback_(true);
this.togglePlaceholder(false);
} else {
throw error;
}
}
}


AMP.extension(TAG, '0.1', AMP => {
AMP.registerElement(TAG, AmpList);
});
20 changes: 15 additions & 5 deletions extensions/amp-list/0.1/test/test-amp-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ describes.realWin('amp-list component', {
});
});

describe('Viewer render template', () => {
// TODO(alabiaga): Fix failing tests.
describe.skip('Viewer render template', () => {
it('should proxy rendering to viewer', () => {
const resp = {data: '<div>Rendered template</div>'};
const itemElement = doc.createElement('div');
Expand Down Expand Up @@ -286,7 +287,7 @@ describes.realWin('amp-list component', {
});
});

// TODO(#14772): Flaky.
// TODO(choumx, #14772): Flaky.
it.skip('should only process one result at a time for rendering', () => {
const doRenderPassSpy = sandbox.spy(list, 'doRenderPass_');
const scheduleRenderSpy = sandbox.spy(list.renderPass_, 'schedule');
Expand Down Expand Up @@ -362,15 +363,17 @@ describes.realWin('amp-list component', {
});
});

it('should fail to load b/c data array is absent', () => {
// TODO: This test passes but causes all following tests to be ignored.
Copy link
Author

Choose a reason for hiding this comment

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

@rsimha Are async throws still an issue? We may be silently skipping other tests too.

Copy link
Contributor

@rsimha rsimha Jul 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@choumx Seems like there's an uncaught error originating from this test. I don't know if this is an async throw (rethrowAsync is no longer asynchronous during tests). I suspect it's a case of #15658 being triggered.

Note: This sort of silent skipping should only happen during local development, where we surface unhandled errors.

Edit: I'm working on a more general fix.

Copy link
Author

Choose a reason for hiding this comment

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

amp-list doesn't use rethrowAsync but it does throw errors asynchronously in some cases.

it.skip('should fail to load b/c data array is absent', () => {
listMock.expects('fetch_').returns(Promise.resolve({})).once();
listMock.expects('toggleLoading').withExactArgs(false).once();
templatesMock.expects('findAndRenderTemplateArray').never();
return expect(list.layoutCallback()).to.eventually.be
.rejectedWith(/Response must contain an array/);
});

it('should fail to load b/c data single-item object is absent', () => {
// TODO: This test passes but causes all following tests to be ignored.
it.skip('should fail to load b/c data single-item object is absent', () => {
element.setAttribute('single-item', 'true');
listMock.expects('fetch_').returns(Promise.resolve()).once();
listMock.expects('toggleLoading').withExactArgs(false).once();
Expand Down Expand Up @@ -489,6 +492,9 @@ describes.realWin('amp-list component', {
expect(list.container_.contains(foo)).to.be.true;

listMock.expects('fetchList_').never();
// Expect hiding of placeholder/loading after render.
listMock.expects('togglePlaceholder').withExactArgs(false).once();
listMock.expects('toggleLoading').withExactArgs(false).once();

element.setAttribute('src', 'https://new.com/list.json');
list.mutatedAttributesCallback({'src': items});
Expand Down Expand Up @@ -520,9 +526,13 @@ describes.realWin('amp-list component', {
return list.layoutCallback().then(() => {
expect(list.container_.contains(foo)).to.be.true;

// Expect display of placeholder/loading but no fetch.
listMock.expects('fetchList_').never();
// Expect display of placeholder/loading before render.
listMock.expects('togglePlaceholder').withExactArgs(true).once();
listMock.expects('toggleLoading').withExactArgs(true, true).once();
// Expect hiding of placeholder/loading after render.
listMock.expects('togglePlaceholder').withExactArgs(false).once();
listMock.expects('toggleLoading').withExactArgs(false).once();

element.setAttribute('src', 'https://new.com/list.json');
list.mutatedAttributesCallback({'src': items});
Expand Down
16 changes: 14 additions & 2 deletions extensions/amp-selector/0.1/test/test-selector-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,17 @@ import {AmpList} from '../../../amp-list/0.1/amp-list';
import {AmpMustache} from '../../../amp-mustache/0.1/amp-mustache';
import {AmpSelector} from '../amp-selector';
import {Deferred} from '../../../../src/utils/promise';
import {Services} from '../../../../src/services';
import {createCustomEvent} from '../../../../src/event-helper';
import {poll} from '../../../../testing/iframe';

// TODO(kevinkassimo): These tests appear to be failing at HEAD.
describes.realWin('amp-selector amp-list interaction', {
amp: {
extensions: ['amp-list', 'amp-selector', 'amp-mustache'],
},
}, function(env) {
let sandbox;
let win, doc, ampdoc;
let parent;
let selector;
Expand All @@ -41,6 +44,7 @@ describes.realWin('amp-selector amp-list interaction', {
beforeEach(() => {
win = env.win;
win.AMP.registerTemplate('amp-mustache', AmpMustache);
sandbox = env.sandbox;

doc = win.document;
ampdoc = env.ampdoc;
Expand All @@ -52,9 +56,11 @@ describes.realWin('amp-selector amp-list interaction', {
parent.addEventListener(AmpEvents.DOM_UPDATE, () => {
updateDeferred.resolve();
});
const selectorElement = doc.createElement('div');
const selectorElement = doc.createElement('amp-selector');
selectorElement.getAmpDoc = () => ampdoc;

Services.resourcesForDoc(ampdoc).add(selectorElement);

AmpSelector.prototype.init_ = sandbox.spy(AmpSelector.prototype, 'init_');
const origMaybeRefreshOnUpdate =
AmpSelector.prototype.maybeRefreshOnUpdate_;
Expand All @@ -65,17 +71,23 @@ describes.realWin('amp-selector amp-list interaction', {
}
};
selector = new AmpSelector(selectorElement);
selectorElement.connectedCallback();
parent.appendChild(selector.element);
selector.buildCallback();

listElement = doc.createElement('div');
listElement = doc.createElement('amp-list');
listElement.setAttribute('src', '/list.json');
listElement.setAttribute('layout', 'responsive');
listElement.setAttribute('width', '100');
listElement.setAttribute('height', '100');
listElement.getAmpDoc = () => ampdoc;
listElement.getFallback = () => null;
listElement.toggleLoading = () => null;
listElement.togglePlaceholder = () => null;
listElement.getResources = () => win.services.resources.obj;

Services.resourcesForDoc(ampdoc).add(listElement);

templateElement = doc.createElement('template');
templateElement.setAttribute('type', 'amp-mustache');
templateElement.innerHTML = '<div option="{{value}}">{{text}}</div>';
Expand Down