Some small UX fixes for the landing page and dialogs #368

netpro2k · 2018-05-04T23:27:12Z

Close dialogs on escape (fixes modal windows should close upon pressing the Esc key or clicking outside the modal #313)
Darken hero video (fixes contrast too bright on video background #305)
Surface ToS and Privacy Policy in help (Fixes Add Privacy Notice + ToS to bottom of Help dialog #315)

cvan · 2018-05-07T23:24:47Z

nit: I notice that when a modal is opened, the background is scrollable vertically. (I'd add a style like this for when a modal is open: body { overflow: hidden; pointer-events: none; })

I can file this in a follow-up issue, if you'd like. not a biggie.

cvan

looks great in Firefox. just need to fix the keypress event for Chrome (and see my few nits).

many good enhancements here. nice work!

cvan · 2018-05-04T23:28:35Z

src/assets/stylesheets/info-dialog.scss

+        justify-content: center;
+        margin: 16px 0;
+        a {
+          margin: 0px 12px;


nit: 0px -> 0

cvan · 2018-05-04T23:30:03Z

src/react-components/info-dialog.js

@@ -211,6 +226,18 @@ class InfoDialog extends Component {
            <p>
              The <b>Bubble Toggle</b> hides avatars that enter your personal space.
            </p>
+            <div className="dialog__box__contents__links">


can this be a <p> or <ul>?

"everythign is a div" is a hard habbit to break hehe.

cvan · 2018-05-04T23:30:47Z

src/assets/stylesheets/index.scss

@@ -45,7 +45,7 @@ body {
  position: fixed;
  top: 0;
  left: 0;
-  opacity: 0.66;
+  opacity: 0.45;


👍 perfect!

cvan · 2018-05-04T23:31:20Z

src/assets/stylesheets/info-dialog.scss

@@ -3,7 +3,7 @@
  height: 100%;
  top: 0;
  left: 0;
-  position: absolute;
+  position: fixed;


👍 nice, good job fixing this - was on my mental laundry list to file

see my comment here: #368 (comment)

cvan · 2018-05-07T23:17:14Z

src/react-components/info-dialog.js

+  }
+
+  onKeyPress(e) {
+    if (e.key === "Escape") {


can you check document.activeElement.matches("body, a")? (or you can check e.target too. there are a couple ways to test this. the a is because the original footer link still has focus when the click event has fired to open the modal but the user has not interacted with the page/modal. usually, document.activeElement would be document.body.)

in the Get Updates modal, when a user has focused the <input> field for the email address, pressing the Escape key should not close the dialog.

Hmm, I am not sure about that one. I would still expect the modal to close even if I was in the textbox if I hit excape. Checking for the "a" doesnt seem right because I could click somewhere inside the dialog and then no longer have an "a" focused.

for accessibility reasons, we try to not hijack keys when form fields are focused. in past projects, we do something like this.

if it's not too much trouble, can you add a tiny helper function to handle this?

var formTags = /input|keygen|meter|option|output|progress|select|textarea/i; function fieldFocused (e) { return formTags.test(e.target.nodeName); }

and then use as such:

if (!fieldFocused(e.target) && e.key === "Escape") {

cvan · 2018-05-07T23:18:44Z

src/react-components/info-dialog.js

+  }
+
+  componentDidMount() {
+    window.addEventListener("keypress", this.onKeyPress);


can you change this to keydown? it doesn't work in Chrome (tested on macOS).

the keypress event does fire correctly in both Firefox and Safari. in Chrome, it turns out that the keypress event is emitted for character keys, but evidently not for such keys as Escape, ArrowDown, etc.

cvan · 2018-05-07T23:29:47Z

src/react-components/info-dialog.js

+
+  onKeyPress(e) {
+    if (e.key === "Escape") {
+      this.props.onCloseDialog();


also, per issue #313 – … or clicking outside the modal – can you also handle that case? I see that a common convention used for both desktop and touch/mobile sites/apps.

cvan

thanks for making the changes! noticed just a few more items. let me know, and I'll re-review it ASAP

cvan · 2018-05-08T17:25:14Z

src/react-components/info-dialog.js

+  }
+
+  componentDidMount() {
+    window.addEventListener("keydown", this.onKeyDown);


can you add document.activeElement.blur(); above this line?

(this will reset the focus to document.body, which is what we'd expect from clicking a modal IMO)

cvan · 2018-05-08T21:14:50Z

src/react-components/info-dialog.js

+  }
+
+  onKeyPress(e) {
+    if (e.key === "Escape") {


for accessibility reasons, we try to not hijack keys when form fields are focused. in past projects, we do something like this.

if it's not too much trouble, can you add a tiny helper function to handle this?

var formTags = /input|keygen|meter|option|output|progress|select|textarea/i; function fieldFocused (e) { return formTags.test(e.target.nodeName); }

and then use as such:

if (!fieldFocused(e.target) && e.key === "Escape") {

cvan · 2018-05-08T21:19:27Z

src/assets/stylesheets/info-dialog.scss

@@ -3,8 +3,7 @@
  height: 100%;
  top: 0;
  left: 0;
-  position: absolute;
-  pointer-events: none;
+  position: fixed;


I notice the <body> scrolling issue (from my GIF) still remains - want me to file a follow-up issue for that?

Yep. I mentioned in slack, the rest of these fixes are clearly minimal impact but I could imagine that one having unintended side-effects since it involves putting pointer-events: none on the body

I missed your message, thanks - yeah

cvan · 2018-05-08T21:27:51Z

src/react-components/info-dialog.js

+  }
+
+  onContainerClicked(e) {
+    if (e.currentTarget === e.target) {


I noticed that the Help dialog from a Hub space (e.g., on https://localhost:8080/hub.html?hub_id=0mnu4u5z3q2) does not get closed when clicking outside. can you change this to if (e.target.closest(".dialog-overlay")) {? (and likely need to move the pointer-events: none style.)

netpro2k added 3 commits May 4, 2018 15:54

Close modals on escape. Fixes #313

d95a21c

Darken intro video. Fixes #305

06e41a3

Surface TOS and prvacy policy in help dialog. Fixes #315

5d9334c

cvan self-requested a review May 7, 2018 23:24

cvan suggested changes May 7, 2018

View reviewed changes

cvan reviewed May 7, 2018

View reviewed changes

Fix exc in chrome, allow clicking outside modal to clsoe, other PR nits

9d7a3f3

cvan suggested changes May 8, 2018

View reviewed changes

netpro2k merged commit 0cdf8bf into master May 9, 2018

netpro2k deleted the feature/home-ux-fixes branch August 21, 2018 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some small UX fixes for the landing page and dialogs #368

Some small UX fixes for the landing page and dialogs #368

netpro2k commented May 4, 2018

cvan commented May 7, 2018

cvan left a comment

cvan May 4, 2018

cvan May 4, 2018

netpro2k May 8, 2018

cvan May 4, 2018

cvan May 4, 2018

cvan May 7, 2018

cvan May 7, 2018 •

edited

Loading

netpro2k May 8, 2018

cvan May 8, 2018

cvan May 7, 2018

cvan May 7, 2018

cvan left a comment

cvan May 8, 2018

cvan May 8, 2018

cvan May 8, 2018

cvan May 8, 2018

netpro2k May 9, 2018

cvan May 9, 2018

cvan May 8, 2018

Some small UX fixes for the landing page and dialogs #368

Some small UX fixes for the landing page and dialogs #368

Conversation

netpro2k commented May 4, 2018

cvan commented May 7, 2018

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

cvan May 7, 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

Choose a reason for hiding this comment

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

cvan May 7, 2018 •

edited

Loading