Send to device follow-ups #393

gfodor · 2018-05-16T21:25:53Z

This PR:

Fixes layout for the code entry screen on iOS.
Properly disables (via a hack) pinch-to-zoom and double-tap-to-zoom on iOS devices for the link page and the hub page. (Apple no longer supports the viewport method in iOS 10+ apparently :P)
Adds a "Have a link code?" link to the homepage. (Note: the reason it's a "link code" and not an "entry code" or a "room code" is because it is unique per "device linking", and shouldn't be considered a code for the room itself or for the ability to enter the room, since that would result in a bad mental model and might imply that users should share the code, even though they are one-time-use and meant for the same user.)
Changes code input type from number to tel because it can have leading zeros.
Adds better error handling for code entry failures.
Fixes up entry flow for Oculus Browser, so that the user is only prompted to "Enter in VR" or "Send to Device"

…ead of number for leading zeros

cvan

looking good! nice adjustments 👍

cvan · 2018-05-18T18:11:19Z

src/assets/stylesheets/index.scss

@@ -57,16 +57,20 @@ body {
  min-height: 90px;
  height: 90px;
  display: flex;
-  border-bottom: 2px solid #242424;
+  border-bottom: 1px solid #242424;


nit: this is rgb(36, 36, 36), which is slightly darker than $darker-grey is. do you want to make a $darkest-grey, heh?

cvan · 2018-05-18T18:12:06Z

src/assets/stylesheets/index.scss

    display: flex;
+    width: 350px;


do you want to make this a variable?

cvan · 2018-05-18T18:13:51Z

src/assets/stylesheets/index.scss

    color: $grey-text;
    font-size: 1.0em;
    font-weight: lighter;
+    white-space: nowrap;


nit; want to do text-overflow: ellipsis;? (I've been meaning to file an issue for this anyway; ellided text that looks truncated tends to look better/natural with … IMO)

i don't think this will ever overflow because once the width is reduced we just stop showing it altogether

cvan · 2018-05-18T18:14:19Z

src/assets/stylesheets/index.scss

+    display: none;
+  }
+
+  padding: 0.5em;


nit: intentional usage of em instead of rem or px?

yeah px seems better

cvan · 2018-05-18T18:15:04Z

src/assets/stylesheets/index.scss

+  }
+
+  padding: 0.5em;
+  background-color: rgba(0, 0, 0, 0.85);


can you use $darkest-transparent?

yep, refactored and flipped everything to 0.9 alpha

cvan · 2018-05-18T18:24:14Z

src/react-components/home-root.js

@@ -132,6 +139,13 @@ class HomeRoot extends Component {
                </div>
              </div>
            </div>
+            <div className="header-subtitle">
+              <div>
+                <a className="header-subtitle__link" href="/link">


rel="nofollow"?

cvan · 2018-05-18T18:30:42Z

src/react-components/link-root.js

@@ -106,7 +111,8 @@ class LinkRoot extends Component {
              <div className={styles.enteredDigits}>
                <input
                  className={styles.digitInput}
-                  type="number"
+                  type="tel"
+                  pattern="[0-9]*"


feels awkward to use type="tel", but then, given the bug in Firefox, and seeing how input[type="tel"]'s mobile entry flow is more PIN-code-like, this is probably sufficient.

do you know how this works with internationalisation (i.e., non-Arabic numerals)?

is Firefox the only browser affected in your experience? Chrome, Safari seemed fine in my testing.

can you add a comment somewhere linking to this relevant Firefox bug?

ah interesting, i didn't realize this was considered a bug. it seemed logical that a "0001" entry for a number seems wrong. i'll add a comment. not sure about internationalisation. even previously it was just a rendering issue, if they just punch the value in the keypad as you'd expect i think it will work no matter what.

cvan · 2018-05-18T18:34:50Z

src/utils/disable-ios-zoom.js

+  let lastTouchAtMs = 0;
+
+  document.addEventListener("touchmove", ev => {
+    if (ev.scale === 1) return;


scale seems to be a WebKit-specific property, not a standardised property of TouchEvent. is this correct?

yes, this is an iOS specific hack since iOS 11 disabled recognizing user-scalable=no

cvan · 2018-05-18T18:37:23Z

src/utils/disable-ios-zoom.js

+    ev.preventDefault();
+  });
+
+  document.addEventListener("touchend", ev => {


do you want to handle the touchforcechange event?

not sure, I don't have a device to test :/ I'll do a fast-follow if this doesn't resolve the issue we were seeing (mis-zooming on the keypad page)

cvan · 2018-05-18T19:15:29Z

src/utils/disable-ios-zoom.js

+
+  document.addEventListener("touchend", ev => {
+    const now = new Date().getTime();
+    const isDoubleTouch = now - lastTouchAtMs <= 300;


I'm pretty sure, but not certain, you could use iOS' gesturestart/gesturechange/gestureend events.

from Safari's Handling Events docs:

To disable pinch open and pinch close gestures in iOS 2.0, implement the gesturestart and gesturechange event handlers as follows:

function gestureChange(event) { // Disable browser zoom event.preventDefault(); }

(there's also dblclick but that may fire too late.)

yeah the double tapping is the main issue for the link page, and the zooming is the main issue for the hub page. this implementation "cleanly" fixes both (I don't like it either) :/

gfodor added 8 commits May 16, 2018 12:33

Fix layout on iOS, prevent zoom on iOS 10+, fix input to use tel inst…

361fbc6

…ead of number for leading zeros

Tweak button size slightly to increase margin on phone screens

373703d

Report failures to sentry of code entry issues

f92237f

Fix up entry choices when in a VR browser already

6c9e9a8

Show link for link code on homepage

31293a9

Fix for flex for entry link link

6b423f9

Fix up CSS

8dca8aa

Ensure link code link is centered

40a8b57

gfodor requested a review from brianpeiris May 16, 2018 21:25

gfodor added 3 commits May 16, 2018 14:35

Better label for send to device in VR

9f5eca3

Hide loader on any errors

8a4328f

Comments and refactoring

db8493e

gfodor requested a review from cvan May 16, 2018 21:45

cvan suggested changes May 18, 2018

View reviewed changes

Cleanup from PR

d67e70b

gfodor merged commit 9c32525 into master May 21, 2018

brianpeiris deleted the feature/device-xfer-follow branch September 24, 2018 17:29

snyk-bot mentioned this pull request Aug 19, 2022

[Snyk] Fix for 30 vulnerabilities MarcelRaschke/hubs#13

Open

MarcelRaschke mentioned this pull request Aug 19, 2022

[Snyk] Fix for 38 vulnerabilities MarcelRaschke/hubs#14

Open

This was referenced Aug 19, 2022

[Snyk] Fix for 30 vulnerabilities devcode1981/hubs#7

Open

[Snyk] Fix for 38 vulnerabilities devcode1981/hubs#8

Open

This was referenced Aug 20, 2022

[Snyk] Fix for 38 vulnerabilities meonBot/hubs#1

Open

[Snyk] Security upgrade copy-webpack-plugin from 4.5.2 to 5.1.2 meonBot/hubs#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send to device follow-ups #393

Send to device follow-ups #393

gfodor commented May 16, 2018 •

edited

Loading

cvan left a comment

cvan May 18, 2018

cvan May 18, 2018

cvan May 18, 2018

gfodor May 21, 2018

cvan May 18, 2018

gfodor May 21, 2018

cvan May 18, 2018

gfodor May 21, 2018

cvan May 18, 2018

cvan May 18, 2018 •

edited

Loading

gfodor May 21, 2018

cvan May 18, 2018 •

edited

Loading

gfodor May 21, 2018

cvan May 18, 2018

gfodor May 21, 2018

cvan May 18, 2018 •

edited

Loading

gfodor May 21, 2018

Send to device follow-ups #393

Send to device follow-ups #393

Conversation

gfodor commented May 16, 2018 • edited Loading

cvan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvan May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvan May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvan May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfodor commented May 16, 2018 •

edited

Loading

cvan May 18, 2018 •

edited

Loading

cvan May 18, 2018 •

edited

Loading

cvan May 18, 2018 •

edited

Loading