Re: [w3c/screen-orientation] Editorial: Example 3 (#135)

marcoscaceres requested changes on this pull request.

Some changes for us to discuss... 

>  
-        var orientationChangeHandler = function() {
-          if (!screen.orientation.type.startsWith('landscape')) {
-            return;
-          }
-          screen.orientation.removeEventListener('change', orientationChangeHandler);
-          startInternal();
+async function start() {
+  try {
+    await screen.orientation.lock("landscape");
+  } catch {
+    const inPortrait = window.matchMedia("(orientation: portrait)");

This seems to assume that the user is initially holding the device in portrait. However, they might already be in landscape mode.  

>  
-        var orientationChangeHandler = function() {
-          if (!screen.orientation.type.startsWith('landscape')) {
-            return;
-          }
-          screen.orientation.removeEventListener('change', orientationChangeHandler);
-          startInternal();
+async function start() {
+  try {
+    await screen.orientation.lock("landscape");
+  } catch {
+    const inPortrait = window.matchMedia("(orientation: portrait)");
+    function orientationChangeListener() {
+      function detectOrientation(e) {
+        if (e.matches) {

Similarly here, it could be that other orientations will match, not just "portrait"... so this should actually be checking for "matches === landscape". 



>          }
-
-        screen.orientation.addEventListener('change', orientationChangeHandler);
-      });
+        window.removeEventListener(
+          "orientationchange",
+          orientationChangeListener
+        );
+        inPortrait.removeListener(detectOrientation);
+        manualRotation();
+      }
+      inPortrait.addListener(detectOrientation);
+    }
+    window.addEventListener("orientationchange", orientationChangeListener);
+    alert("To start, please rotate your screen to landscape");

Squishing all of the above, I get to:

```JS
function start() {
  /* Start application when in correct orientation */
}
async function rotate() {
  try {
    await screen.orientation.lock("landscape");
    start();
  } catch (err) {
    console.error(err);
  }
  // Not supported or not working, so...
  const matchLandscape = matchMedia("(orientation: landscape)");
  if (matchLandscape.matches) return start(); // Already landscape!
  // Let's get the user to rotate manually...
  addEventListener("orientationchange", function listener() {
    matchPortrait.addListener(function mediaChange(e) {
      if (!e.matches) return; // Nope, not landscape!
      removeEventListener("orientationchange", listener);
      matchPortrait.removeListener(mediaChange);
      start();
    });
  });
  alert("To start, please rotate your screen to landscape.");
}
```

>  <script>
-  var start = function() {
-    screen.orientation.lock('landscape-primary').then(
-      startInternal,
-      function() {
-        alert('To start, rotate your screen to landscape.');
+function manualRotation() {

I think we should just inline this (see below). 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/screen-orientation/pull/135#pullrequestreview-194823488