Skip to content

Mobile full screen example #632

Merged
merged 2 commits into from Apr 21, 2013

5 participants

@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
OpenLayers 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
OpenLayers member

Looks good to me.

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

@twpayne
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
OpenLayers 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 twpayne merged commit e6f354c into openlayers:master Apr 21, 2013

1 check passed

Details default The Travis build passed
@twpayne twpayne deleted the twpayne:mobile-full-screen-example branch Apr 21, 2013
@fredj
OpenLayers 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.