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

Clean up: Move viewport specific logic out from viewer. #5887

Merged
merged 4 commits into from Nov 1, 2016

Conversation

lannka
Copy link
Contributor

@lannka lannka commented Oct 28, 2016

No description provided.

let viewportType = viewer.getParam('viewportType') || ViewportType.NATURAL;
if (platformFor(win).isIos()
&& ((viewportType == ViewportType.NATURAL && viewer.isIframed())
// Enable iOS Embedded mode so that it's easy to test against a more
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's' move this comment to the assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the comment is specific to dev mode, so better be here

@cramforce cramforce assigned dvoytenko and unassigned cramforce Oct 28, 2016
@lannka lannka merged commit 82d2921 into ampproject:master Nov 1, 2016
@lannka lannka deleted the viewer_viewport branch November 1, 2016 21:02
lannka added a commit to lannka/amphtml that referenced this pull request Nov 2, 2016
…: Move viewport specific logic out from viewer.)
lannka added a commit that referenced this pull request Nov 2, 2016
@dvoytenko
Copy link
Contributor

@lannka To confirm, did you catch up this PR to @cramforce's changes?

@cramforce
Copy link
Member

Ugh, I didn't check for it and this PR doesn't have the extra check. Means our canary has the bad state again. @aghassemi

@dvoytenko
Copy link
Contributor

Ok. Let's start with fixing it. @lannka Could you please send the fix? Or should we rollback?

@aghassemi
Copy link
Contributor

@dvoytenko false alarm. We checked master and Malte's code is there as part of #5972

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
…: Move viewport specific logic out from viewer.) (ampproject#5972)
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
…: Move viewport specific logic out from viewer.) (ampproject#5972)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants