Re: [w3c/screen-orientation] Editorial: Examples section edits (#137)

marcoscaceres requested changes on this pull request.

Looking good, but example 1 needs a bit of a rework... 

> +  <script>
+  function fullScreenCheck() {
+    if (document.fullscreenElement) return;
+    return document.documentElement.requestFullscreen();
+  }
+  
+  const lockButton = document.getElementById("lockBtn");
+
+  function btnOrientation() {
+    const btnOrientation = oppOrientation();
+    lockButton.textContent = `Lock to ${btnOrientation}`;
+  }
+  
+  function oppOrientation() {
+    const { type } = screen.orientation;
+    if (type.startsWith("portrait")) {

Nit: maybe just... 
```JS
return type.startsWith("portrait") ? "landscape" : "portrait";
```

> +    const newOrientation = oppOrientation();
+    await screen.orientation.lock(newOrientation);
+    btnOrientation();
+  }
+  
+  function show() {
+    const { type, angle } = screen.orientation;
+    console.log(`Orientation type is ${type} & angle is ${angle}.`);
+  }
+  
+  screen.orientation.addEventListener("change", show);
+  screen.orientation.addEventListener("change", btnOrientation);
+  window.addEventListener("load", show);
+  </script>
+  
+  <button onclick="rotate() id="lockBtn">

If we pass "this", then we don't need to look up the button. 
```suggestion
  <button onclick="rotate(this)">
```

> +  const lockButton = document.getElementById("lockBtn");
+
+  function btnOrientation() {
+    const btnOrientation = oppOrientation();
+    lockButton.textContent = `Lock to ${btnOrientation}`;
+  }
+  
+  function oppOrientation() {
+    const { type } = screen.orientation;
+    if (type.startsWith("portrait")) {
+      return "landscape";
+    }
+    return "portrait";
+  }
+
+  async function rotate() {

See below... we can just pass the button that was clicked.  
```suggestion
  async function rotate(rotateBtn) {
```

> +
+  function btnOrientation() {
+    const btnOrientation = oppOrientation();
+    lockButton.textContent = `Lock to ${btnOrientation}`;
+  }
+  
+  function oppOrientation() {
+    const { type } = screen.orientation;
+    if (type.startsWith("portrait")) {
+      return "landscape";
+    }
+    return "portrait";
+  }
+
+  async function rotate() {
+    const rotateBtn = document.getElementById("rotateBtn");

See below... 
```suggestion

```

> +    try {
+      await fullScreenCheck();
+    } catch (err) {
+      console.error(err);
+    }
+    const newOrientation = oppOrientation();
+    await screen.orientation.lock(newOrientation);
+    btnOrientation();
+  }
+  
+  function show() {
+    const { type, angle } = screen.orientation;
+    console.log(`Orientation type is ${type} & angle is ${angle}.`);
+  }
+  
+  screen.orientation.addEventListener("change", show);

combine these two into a singe
```JS
addEventListener("change", ()=>{
   show();
   btnOrientation(); 
}) 
```
That also assures the order means that the browser doesn't need to fire two separate events.

> +            unlocks to its default orientation.
+          </p>
+          <p>
+            The developer console logs the change in orientation type and
+            angle.
+          </p>
+          <pre class='example html'>
+  &lt;script&gt;
+  function fullScreenCheck() {
+    if (document.fullscreenElement) return;
+    return document.documentElement.requestFullscreen();
+  }
+  
+  const lockButton = document.getElementById("lockBtn");
+
+  function btnOrientation() {

Nit - make sure every functions name is a verb (and clearly indicates what action it does), and not a noun. Also, it's always best to pass things into a function, and not rely on the global scope. 
  
```suggestion
  function updateDetails(lockButton) {
```



> +          </p>
+          <pre class='example html'>
+  &lt;script&gt;
+  function fullScreenCheck() {
+    if (document.fullscreenElement) return;
+    return document.documentElement.requestFullscreen();
+  }
+  
+  const lockButton = document.getElementById("lockBtn");
+
+  function btnOrientation() {
+    const btnOrientation = oppOrientation();
+    lockButton.textContent = `Lock to ${btnOrientation}`;
+  }
+  
+  function oppOrientation() {

As above with the name... maybe `getOppositeOrientation`. Generally, try to avoid abbreviations in names (applies globally as a comment).    

> +            into fullscreen and then lock to the opposite orientation. If the
+            screen is not in default orientation, pressing the "Unlock" button
+            unlocks to its default orientation.
+          </p>
+          <p>
+            The developer console logs the change in orientation type and
+            angle.
+          </p>
+          <pre class='example html'>
+  &lt;script&gt;
+  function fullScreenCheck() {
+    if (document.fullscreenElement) return;
+    return document.documentElement.requestFullscreen();
+  }
+  
+  const lockButton = document.getElementById("lockBtn");

This will be "racy" because lockButton doesn't actually exist yet (the HTML parser has not yet parsed button underneath). So this will be null.  

I'd suggest deleting it, and query the doc only when you need this. 

```suggestion

```

> +          <p>
+            The developer console logs the change in orientation type and
+            angle.
+          </p>
+          <pre class='example html'>
+  &lt;script&gt;
+  function fullScreenCheck() {
+    if (document.fullscreenElement) return;
+    return document.documentElement.requestFullscreen();
+  }
+  
+  const lockButton = document.getElementById("lockBtn");
+
+  function btnOrientation() {
+    const btnOrientation = oppOrientation();
+    lockButton.textContent = `Lock to ${btnOrientation}`;

I think this is not correct. As we discussed yesterday, `.unlock()` doesn't mean it will revert the orientation. Thus, the `lockButton`'s text should always just be "Unlock". However, the "Rotate" button should change its text... 

-- 
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/137#pullrequestreview-195844630