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

Send to device follow-ups #393

Merged
merged 12 commits into from
May 21, 2018
48 changes: 46 additions & 2 deletions src/assets/stylesheets/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,20 @@ body {
min-height: 90px;
height: 90px;
display: flex;
border-bottom: 2px solid #242424;
border-bottom: 1px solid #242424;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


&__title {
flex: 10;
display: flex;
width: 350px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to make this a variable?


@media (max-width: 768px) {
justify-content: center;
}

@media (max-width: 1024px) {
flex: 1 1 350px;
}

&__name {
width: 200px;
}
Expand All @@ -77,12 +81,32 @@ body {
}
}

&__entry-code {
@media (max-width: 1024px) {
display: none;
}

flex: 10;
text-align: center;
display: flex;
flex-direction: row;
justify-content: center;
align-items: center;

&__link {
color: white;
text-decoration-color: $light-grey;
}
}

&__experiment {
text-align: right;
flex: 1 1 350px;
width: 350px;
color: $grey-text;
font-size: 1.0em;
font-weight: lighter;
white-space: nowrap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


@media (max-width: 768px) {
display: none;
Expand Down Expand Up @@ -117,6 +141,26 @@ body {
}
}

.header-subtitle {
@media (min-width: 1024px) {
display: none;
}

padding: 0.5em;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah px seems better

background-color: rgba(0, 0, 0, 0.85);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use $darkest-transparent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, refactored and flipped everything to 0.9 alpha

text-align: center;
display: flex;
flex-direction: row;
justify-content: center;
align-items: center;
border-bottom: 2px solid #242424;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably again makes sense here to use a constant variable (as used above too)


&__link {
color: white;
text-decoration-color: $light-grey;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this 👍

}
}

.hero-content {
flex: 10;
min-height: 740px;
Expand Down
7 changes: 4 additions & 3 deletions src/assets/stylesheets/link.scss
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ a {
display: grid;
grid-template-columns: 1fr 1fr 1fr;
grid-template-rows: 1fr 1fr 1fr 1fr;
text-align: center;
}

:local(.keypad-button) {
Expand All @@ -116,8 +117,8 @@ a {
font-family: sans-serif;
border: 4px $light-grey solid;
border-radius: 128px;
min-width: 88px;
min-height: 88px;
min-width: 80px;
min-height: 80px;
cursor: pointer;
line-height: 68px;
margin: 8px;
Expand Down Expand Up @@ -170,7 +171,7 @@ a {
margin: 0;
font-size: 64pt;
border: 0;
width: 225px;
width: 295px;
letter-spacing: 0.08em;
text-align: center;
}
Expand Down
4 changes: 3 additions & 1 deletion src/assets/translations.data.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"entry.device-medium": "Device",
"entry.device-subtitle-desktop": "Standalone Headset or Phone",
"entry.device-subtitle-mobile": "Mobile Headset or PC",
"entry.device-subtitle-vr": "Phone or PC",
"entry.cardboard": "Enter on Google Cardboard",
"entry.daydream-prefix": "Enter on ",
"entry.daydream-medium": "Daydream",
Expand Down Expand Up @@ -59,10 +60,11 @@
"home.made_with_love": "made with 🦆 by ",
"home.environment_author_by": " by ",
"home.dialog.close": "CLOSE",
"home.have_entry_code": "Have a link code?",
"mailing_list.privacy_label": "I'm okay with Mozilla handling my info as explained in",
"mailing_list.privacy_link": "this Privacy Notice",
"link.in_your_browser": "In your device's browser, go to:",
"link.enter_code": "Then, enter this code:",
"link.enter_code": "Then, enter this link code:",
"link.do_not_close": "Keep this dialog open to use this code.",
"link.link_page_header": "Enter your code:",
"link.dont_have_a_code": "Don't have a code?",
Expand Down
3 changes: 3 additions & 0 deletions src/hub.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ import UIRoot from "./react-components/ui-root";
import HubChannel from "./utils/hub-channel";
import LinkChannel from "./utils/link-channel";
import { connectToReticulum } from "./utils/phoenix-utils";
import { disableiOSZoom } from "./utils/disable-ios-zoom";

import "./systems/personal-space-bubble";
import "./systems/app-mode";
Expand Down Expand Up @@ -133,6 +134,8 @@ if (!isBotMode) {
registerTelemetry();
}

disableiOSZoom();

AFRAME.registerInputBehaviour("trackpad_dpad4", trackpad_dpad4);
AFRAME.registerInputBehaviour("joystick_dpad4", joystick_dpad4);
AFRAME.registerInputBehaviour("msft_mr_axis_with_deadzone", msft_mr_axis_with_deadzone);
Expand Down
1 change: 1 addition & 0 deletions src/link.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
<link rel="shortcut icon" type="image/png" href="/favicon.ico"/>
<title>Enter Code | Hubs by Mozilla</title>
<link href="https://fonts.googleapis.com/css?family=Zilla+Slab:300,300i,400,400i,700" rel="stylesheet">
<meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the usage is appropriate here to make the onboarding feel app-y, but usually on principle, regardless of the UI design, user-scalable=no is considered harmful for accessibility.

can you use the touch-action CSS property?

related reading:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

touch-action seems unsupported on safari, and iOS Safari is the specific browser causing issues with double-tap and pinch-to-zoom. i understand user-scalable=no is considered harmful for accessibility because it limits the user's ability to zoom text -- however in both pages we are using it the concern is basically not relevant. for the main hub page, zooming breaks the app, and for the link entry page, the keypad is visually designed to be high contrast, readable, and accessible. there is a minor amount of text (validation errors, etc) on the link page that arguably could be harmed by not being able to zoom, but given that this is causing a usability issue that is much worse, it seems like a fair tradeoff.

</head>

<body>
Expand Down
10 changes: 7 additions & 3 deletions src/react-components/entry-buttons.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ EntryButton.propTypes = {
iconSrc: PropTypes.string,
prefixMessageId: PropTypes.string,
mediumMessageId: PropTypes.string,
subtitle: PropTypes.string
subtitle: PropTypes.string,
isInHMD: PropTypes.bool
};

export const TwoDEntryButton = props => {
Expand Down Expand Up @@ -91,9 +92,12 @@ export const DeviceEntryButton = props => {
...props,
iconSrc: DeviceEntryImg,
prefixMessageId: mobiledetect.mobile() ? "entry.device-prefix-mobile" : "entry.device-prefix-desktop",
mediumMessageId: "entry.device-medium",
subtitle: mobiledetect.mobile() ? "entry.device-subtitle-mobile" : "entry.device-subtitle-desktop"
mediumMessageId: "entry.device-medium"
};

entryButtonProps.subtitle = entryButtonProps.isInHMD
? "entry.device-subtitle-vr"
: mobiledetect.mobile() ? "entry.device-subtitle-mobile" : "entry.device-subtitle-desktop";

return <EntryButton {...entryButtonProps} />;
};
14 changes: 14 additions & 0 deletions src/react-components/home-root.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,13 @@ class HomeRoot extends Component {
<img className="header-content__title__name" src="../assets/images/logo.svg" />
<div className="header-content__title__preview">preview</div>
</div>
<div className="header-content__entry-code">
<div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these inner <div>s actually necessary? I don't think they are, but lmk

Copy link
Contributor Author

@gfodor gfodor May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep not sure about why that was there

<a className="header-content__entry-code__link" href="/link">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rel="nofollow"?

<FormattedMessage id="home.have_entry_code" />
</a>
</div>
</div>
<div className="header-content__experiment">
<div className="header-content__experiment__container">
<img src="../assets/images/webvr_cube.svg" className="header-content__experiment__icon" />
Expand Down Expand Up @@ -132,6 +139,13 @@ class HomeRoot extends Component {
</div>
</div>
</div>
<div className="header-subtitle">
<div>
<a className="header-subtitle__link" href="/link">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rel="nofollow"?

<FormattedMessage id="home.have_entry_code" />
</a>
</div>
</div>
<div className="hero-content">
<div className="hero-content__attribution">
Medieval Fantasy Book by{" "}
Expand Down
10 changes: 8 additions & 2 deletions src/react-components/link-root.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ import en from "react-intl/locale-data/en";
import { lang, messages } from "../utils/i18n";
import classNames from "classnames";
import styles from "../assets/stylesheets/link.scss";
import { disableiOSZoom } from "../utils/disable-ios-zoom";

const MAX_DIGITS = 4;

addLocaleData([...en]);
disableiOSZoom();

class LinkRoot extends Component {
static propTypes = {
Expand Down Expand Up @@ -78,8 +80,11 @@ class LinkRoot extends Component {
}
})
.catch(e => {
console.error(e);
this.setState({ failedAtLeastOnce: true, enteredDigits: "" });

if (!(e instanceof Error && (e.message === "in_use" || e.message === "failed"))) {
throw e;
}
});
};

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

@cvan cvan May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

value={this.state.enteredDigits}
onChange={ev => {
this.setState({ enteredDigits: ev.target.value });
Expand Down
6 changes: 4 additions & 2 deletions src/react-components/ui-root.js
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,9 @@ class UIRoot extends Component {
this.state.entryStep === ENTRY_STEPS.start ? (
<div className="entry-panel">
<div className="entry-panel__button-container">
<TwoDEntryButton onClick={this.enter2D} />
{this.props.availableVREntryTypes.screen !== VR_DEVICE_AVAILABILITY.no && (
<TwoDEntryButton onClick={this.enter2D} />
)}
{this.props.availableVREntryTypes.generic !== VR_DEVICE_AVAILABILITY.no && (
<GenericEntryButton onClick={this.enterVR} />
)}
Expand All @@ -626,7 +628,7 @@ class UIRoot extends Component {
}
/>
)}
<DeviceEntryButton onClick={this.attemptLink} />
<DeviceEntryButton onClick={this.attemptLink} isInHMD={this.props.availableVREntryTypes.isInHMD} />
{this.props.availableVREntryTypes.cardboard !== VR_DEVICE_AVAILABILITY.no && (
<div className="entry-panel__secondary" onClick={this.enterVR}>
<FormattedMessage id="entry.cardboard" />
Expand Down
24 changes: 24 additions & 0 deletions src/utils/disable-ios-zoom.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import MobileDetect from "mobile-detect";
const mobiledetect = new MobileDetect(navigator.userAgent);

export function disableiOSZoom() {
if (!mobiledetect.is("iPhone") && !mobiledetect.is("iPad")) return;

let lastTouchAtMs = 0;

document.addEventListener("touchmove", ev => {
if (ev.scale === 1) return;
Copy link
Contributor

@cvan cvan May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ev.preventDefault();
});

document.addEventListener("touchend", ev => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to handle the touchforcechange event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

const now = new Date().getTime();
const isDoubleTouch = now - lastTouchAtMs <= 300;
Copy link
Contributor

@cvan cvan May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) :/

lastTouchAtMs = now;

if (isDoubleTouch) {
ev.preventDefault();
}
});
}
12 changes: 9 additions & 3 deletions src/utils/vr-caps-detect.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,27 @@ const GENERIC_ENTRY_TYPE_DEVICE_BLACKLIST = [/cardboard/i];
// Once in a compatible browser, we should assume it will work (if it doesn't, it's because they don't have the headset,
// haven't installed the software, our guess about their phone was wrong, etc.)
//
// At the time of this writing there are three VR "entry types" that will be validated by this method:
// At the time of this writing there are the following VR "entry types" that will be validated by this method:
//
// - screen: Enter "on-screen" in 2D
// - generic: Generic WebVR (platform/OS agnostic indicator if a general 'Enter VR' option should be presented.)
// - daydream: Google Daydream
// - gearvr: Oculus GearVR
// - cardboard: Google Cardboard
//
// This function also detects if the user is already in a headset, and returns the isInHMD key to be `true` if so.
export async function getAvailableVREntryTypes() {
const isSamsungBrowser = browser.name === "chrome" && navigator.userAgent.match(/SamsungBrowser/);
const isOculusBrowser = navigator.userAgent.match(/Oculus/);
const isInHMD = isOculusBrowser;

// This needs to be kept up-to-date with the latest browsers that can support VR and Hubs.
// Checking for navigator.getVRDisplays always passes b/c of polyfill.
const isWebVRCapableBrowser = window.hasNativeWebVRImplementation;

const isDaydreamCapableBrowser = !!(isWebVRCapableBrowser && browser.name === "chrome" && !isSamsungBrowser);

const screen = isInHMD ? VR_DEVICE_AVAILABILITY.no : VR_DEVICE_AVAILABILITY.yes;
let generic = mobiledetect.mobile() ? VR_DEVICE_AVAILABILITY.no : VR_DEVICE_AVAILABILITY.maybe;
let cardboard = VR_DEVICE_AVAILABILITY.no;

Expand All @@ -65,7 +70,8 @@ export async function getAvailableVREntryTypes() {

// For daydream detection, we first check if they are on an Android compatible device, and assume they
// may support daydream *unless* this browser has WebVR capabilities, in which case we can do better.
let daydream = isMaybeDaydreamCompatibleDevice() ? VR_DEVICE_AVAILABILITY.maybe : VR_DEVICE_AVAILABILITY.no;
let daydream =
isMaybeDaydreamCompatibleDevice() && !isInHMD ? VR_DEVICE_AVAILABILITY.maybe : VR_DEVICE_AVAILABILITY.no;

if (isWebVRCapableBrowser) {
const displays = await navigator.getVRDisplays();
Expand Down Expand Up @@ -94,5 +100,5 @@ export async function getAvailableVREntryTypes() {
}
}

return { generic, gearvr, daydream, cardboard };
return { screen, generic, gearvr, daydream, cardboard, isInHMD };
}