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

Mobile full screen example #632

Merged
merged 2 commits into from Apr 21, 2013

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Apr 20, 2013

This PR adds a full screen example suitable for devices that don't support the Full Screen API, e.g. mobile devices and IE.

@marcjansen
Copy link
Member

Hi @twpayne, this looks good and can be merged.

I would have preferred a separate PR for the geolocation changes as these aren't directly related. Feel free to merge this, though.

@elemoine
Copy link
Member

Looks good to me.

Personal opinion: it's more important to have separate commits than separate PRs.

@twpayne
Copy link
Contributor Author

twpayne commented Apr 20, 2013

I would have preferred a separate PR for the geolocation changes as these aren't directly related. Feel free to merge this, though.

I would as well. The changes are related because ddb295c depends on be7228f (link). Trying to maintain separate PRs gets really complicated really quickly. If you don't mind commits taking 2 days (or more) instead of 2 minutes to be reviewed then it's OK that development should be slowed down by 2 * 24 * 60 / 2 = 1440 times. We all have years to spare, right? At least separating commits means that the changes are clear and we can still merge the non-controversial improvements.

@ahocevar
Copy link
Member

If you don't mind commits taking 2 days (or more) instead of 2 minutes to be reviewed then it's OK that development should be slowed down by 2 * 24 * 60 / 2 = 1440 times.

This comment is not really encouraging to reviewers. One thing I've learned in 6 years of OpenSource community work is that patience is a virtue when it comes to waiting for code to get reviewed. And sometime the code changes and improves while waiting, because of the extra time you get to think about it. Not all bad.

twpayne added a commit that referenced this pull request Apr 21, 2013
@twpayne twpayne merged commit e6f354c into openlayers:master Apr 21, 2013
@twpayne twpayne deleted the mobile-full-screen-example branch April 21, 2013 16:46
@fredj
Copy link
Member

fredj commented Apr 22, 2013

The geolocation recenter does not work on http://ol3js.org/ol3/master/examples/mobile-full-screen.html
geolocation.exports needs to include ol.GeolocationOptions

afabiani pushed a commit to geosolutions-it/openlayers that referenced this pull request May 22, 2017
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