Skip to content

Remove maximum-scale requirement for viewport#620

Merged
dvoytenko merged 3 commits intoampproject:masterfrom
dvoytenko:preservezoom
Oct 14, 2015
Merged

Remove maximum-scale requirement for viewport#620
dvoytenko merged 3 commits intoampproject:masterfrom
dvoytenko:preservezoom

Conversation

@dvoytenko
Copy link
Copy Markdown
Contributor

and add ability to disable it when needed.

Closes #592.

/cc @cramforce

src/viewport.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could we add an explanation why we do this here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

src/viewport.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is it possible to just split on the = ? or is there anything unsafe in doing that (multiple =s ?)

let [name, value] = pair.split('=');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's nice. I'll redo. Things get a little less "pretty" once you start doing trims, etc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

true, i'm ok with this implementation 👍

@erwinmombay
Copy link
Copy Markdown
Member

@dvoytenko LGTM!

dvoytenko added a commit that referenced this pull request Oct 14, 2015
Remove maximum-scale requirement for viewport
@dvoytenko dvoytenko merged commit 6e95277 into ampproject:master Oct 14, 2015
@dvoytenko dvoytenko deleted the preservezoom branch October 14, 2015 21:21
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.

3 participants