[REBASED TO DEATH] Implement show and toggle as standard actions#7658
[REBASED TO DEATH] Implement show and toggle as standard actions#7658alanorozco wants to merge 88 commits intoampproject:masterfrom
Conversation
* adding license * remove newline issue
* initial commit to forbid exprs in obj keys * also support numbers and other primitives
* initial commit for worker blob * fix type error * malte's comments * fix unit tests
Missed copyright in original PR
…t#7660) * Replace CLICK_X and CLICK_Y in anchor href for amp-inabox. * Fix lint errors.
* Send expected errors to expected bucket * Increment errortracker version
| } | ||
|
|
||
| /** | ||
| * Expands the element, resetting its default display value, and notifies its |
There was a problem hiding this comment.
{@link expandedCallback}
src/service/standard-actions-impl.js
Outdated
| * @param {!./action-impl.ActionInvocation} invocation | ||
| */ | ||
| handleToggle(invocation) { | ||
| if (getStyle(dev().assertElement(invocation.target), 'display') == 'none') { |
There was a problem hiding this comment.
we should use getComputed style here so if element is hidden some other way (e.g. author defined CSS class) we toggle properly.
There was a problem hiding this comment.
Done. Using vsync to avoid layout thrashing.
|
|
||
| }); | ||
|
|
||
| describe('"toggle" action', () => { |
There was a problem hiding this comment.
and a test here to handle the comment above about computedStyle. Something along the lines of creating a style tag with a class like .hide { display: none } and using that class instead of display=none.
There was a problem hiding this comment.
Done. Stubbed vsync and added elements to the body tag in the test so that getComputedStyle can work properly.
| standardActions.handleShow({target: element}); | ||
| expect(deferMutateStub).to.be.calledOnce; | ||
| expect(deferMutateStub.firstCall.args[0]).to.equal(element); | ||
| expect(element.expand).to.be.calledOnce; |
There was a problem hiding this comment.
also expectElementToHaveBeenShown(element); here?
There was a problem hiding this comment.
expectElementToHaveBeenShown does not expect an AMP element. Extracted functions for AMP elements.
| }); | ||
|
|
||
| it('should handle AmpElement', () => { | ||
| const element = createAmpElement(); |
There was a problem hiding this comment.
Let's also add a test where an AMP document is hidden using layout=nodisplay and then a show action is called on it ensuring display:none added by layout=nodisplay goes away after the action.
There was a problem hiding this comment.
It looks like using nodisplay sets the display value of the element to none.
To be able to create this test, a test iframe would have to be constructed and then a specific AMP element would have to be installed in the iframe. I'm not sure if it's worth it to add this case since AFAICT the internal implementation would be the same regardless.
There was a problem hiding this comment.
true, nodisplay is equal to elem.style.display=none test case
…ject#7713) * Updated CSS for amp-selector so the outline is less specific. (Issue ampproject#7534) * Added new line at end of file.
* first draft * added details inline; added quick start * added a preface for doc/test/etc. changes * minor fixes * responding to review comments; adding README
…#7619) * initial changes * add test coverage * remove dependency on createIframePromise causing type check failure * build failures * build fix * fix test failure * change AV config from urls to url * PR review * Add scoped attribute to amp-analytics * fix lint error * fix lint error * fix presubmit error
…roject#7709) * Pass nocurls as additional info to Viewer for latency tracking. * Add test. * use urls.cdn * Add comment. * ... * ... * rename experiment name * fix test
…xception (ampproject#7723) * Only use local storage if the API is declared and does not throw an exception (ampproject#6633) * Revert "Only use local storage if the API is declared and does not throw an exception (ampproject#6633)" This reverts commit fb0f11f. * Only use local storage if the API is declared and does not throw an exception (ampproject#6633) * Fix whitespace lint issue * Ensure that getItem is not called when attempting to load an item from local storage when local storage is not supported.
* Detect JS Engine when reporting errors This should save us some time trying to determine what the real browser is, not what the UserAgent is masquerading as. * Fix IE detection Also add comments * linting * JSDoc * Add error tests to integrations suite * Fix closure type check
ampproject#7722) * Doc tweaks for formatting & style + valid layouts * Update amp-call-tracking.md
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
Closing due to rebase issue. New PR is #7785 |
Refer to #7785