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.

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

✨Support different animation styles in amp-lightbox #16813

Merged
merged 10 commits into from Jul 18, 2018

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Jul 17, 2018

Fixes #2374.

@@ -47,6 +47,12 @@ tags: { # <amp-lightbox>
attrs: { name: "controls" }
attrs: { name: "from" }
attrs: { name: "scrollable" }
attrs: {
name: "animate-in"
Copy link
Contributor

Choose a reason for hiding this comment

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

please alphabetize

also, repeated fields for value and value_casei is not on github yet, but we'll try to get it there today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alphabetized.

Waiting for repeated value_casei support to make it into the repo and then will merge.

st.resetStyles(element, ['display']);

this.mutateElement(() => {
element./*OK*/scrollTop = 0;
});

this.handleAutofocus_();

// TODO (jridgewell): expose an API accomodating this per PR #14676
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to use the dirtyElement method here per #14917, right @jridgewell ?

st.setStyles(this.element, {
display: '',
opacity: 0,
st.setStyles(element, dict({
Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion, this shouldn't interfere with anything users could possibly do unless they are styling the transform property themselves. In that case, we need to gracefully bail / emit error message / not do anything, and document any relevant caveats.

Copy link
Member Author

Choose a reason for hiding this comment

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

Documented.

@cathyxz
Copy link
Contributor

cathyxz commented Jul 17, 2018

Actually, can you also verify that the manual test referenced in #14676 still works with the dirtyElement API change? Should be /test/manual/amp-lightbox-focus.amp.html on iOS.

@alanorozco
Copy link
Member Author

@cathyxz

Manual test working just fine :)

@cathyxz
Copy link
Contributor

cathyxz commented Jul 17, 2018

LGTM

@codecov-io
Copy link

codecov-io commented Jul 17, 2018

Codecov Report

Merging #16813 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #16813      +/-   ##
==========================================
- Coverage   78.09%   78.03%   -0.07%     
==========================================
  Files         558      558              
  Lines       40437    40438       +1     
==========================================
- Hits        31578    31554      -24     
- Misses       8859     8884      +25
Flag Coverage Δ
#integration_tests 34.85% <ø> (-0.29%) ⬇️
#unit_tests 77.07% <ø> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ea9bde...6524afd. Read the comment docs.

@alanorozco
Copy link
Member Author

@honeybadgerdontcare

Updated after validator rollup and added validation tests. PTAL.

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

validation changes look good

@alanorozco alanorozco merged commit afd61bf into ampproject:master Jul 18, 2018
@alanorozco alanorozco deleted the lightbox-anim branch July 18, 2018 15:51
cramforce added a commit that referenced this pull request Jul 18, 2018
@aghassemi aghassemi added this to PR - merged in FixIt - UI - July 2018 Jul 20, 2018
@aghassemi
Copy link
Contributor

@alanorozco thanks for taking this over FixIt week! Could you create a demo when it makes it to prod and sent it to the team for a look and feedback?

gopanisandip pushed a commit to gopanisandip/amphtml that referenced this pull request Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
FixIt - UI - July 2018
  
PR - merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants