Enable ESC and clicking background to close full screen dialog #8697

gravityrail · 2018-01-31T21:14:21Z

Changes proposed in this Pull Request:

Clicking the background or pressing the ESC key should close full screen dialogs
Move close button into the dialog body.

Testing instructions:

Trigger a full-screen dialog. This code snippet in a micro-plugin will do the trick for forcing an upgrade prompt on any of the Jetpack dash/settings pages:

add_action( 'plugins_loaded', function() {
    Jetpack::state( 'message', 'modules_activated' );
});

Click the background, the dialog should be closed
Click the foreground (main part of dialog or picture above it) - the dialog should NOT be closed
Re-open the dialog (e.g. by re-loading). Hit the ESC key. The dialog should be closed.

This also moves the close button into the top-right corner of the dialog, and includes an updated gridicons file which includes the circular close icon. Other uses of gridicons should be re-tested to ensure I didn't break anything.

Proposed changelog entry for your changes:

Better usability for full-screen dialogs.

dereksmart · 2018-01-31T21:20:52Z

_inc/client/components/jetpack-dialogue/index.jsx

+
+	// capture the ESC key globally
+	componentDidMount(){
+		document.addEventListener('keydown', this.props.dismiss, false);


I think these new rules should respect this.props.showDismiss -- there may be instances we don't want to allow dismissal without action.

Good idea - fixed

git push? While you're in there can you apply code standard spacing?

dereksmart · 2018-01-31T22:10:13Z

esc is throwing a console error with the recent changes

dereksmart · 2018-01-31T22:11:51Z

Is the update to the gridicon library necessary for this change? I think it will require some more testing. These changes now are modifying existing styles, example:

dereksmart · 2018-01-31T22:15:40Z

yarn lint is throwing some a11y errors causing the tests to fail

joanrho · 2018-01-31T22:23:16Z

@gravityrail can you replace the "cross-circle" Gridicon with either the "cross" or "cross-small" instead?

Here's a screenshot of what the "cross-small" could look like there:

gravityrail · 2018-01-31T22:32:42Z

@joanrho - icon updated, and screenshot in description

@dereksmart - feedback addressed, going to dismiss your review so you can take another look :)

Made some changes

joanrho · 2018-01-31T23:38:44Z

Design review approved. Thanks for the edits, Dan!

dereksmart

LGTM!

zinigor

Made one small change to remove bind calls from properties, otherwise LGTM whenever Travis finishes!

* Enable ESC and clicking background to close full screen dialog * Update gridicons and reposition close button on dialog * Better spacing * Only import the one gridicon we need * Revert unnecessary changes to gridicons * Fix JS error due to unbound handler * Fix event binding and restrict to ESC code * Fix eslint error * Removed bind calls from property assignments.

zinigor · 2018-02-01T19:43:41Z

Ported to branch-5.8 in 36b4d68.

Enable ESC and clicking background to close full screen dialog

3139f1d

gravityrail requested a review from a team as a code owner January 31, 2018 21:14

gravityrail requested a review from dereksmart January 31, 2018 21:14

gravityrail self-assigned this Jan 31, 2018

gravityrail added [Type] Bug When a feature is broken and / or not performing as intended [Focus] Accessibility Improving usability for all users (a11y) labels Jan 31, 2018

gravityrail added this to the 5.8 milestone Jan 31, 2018

gravityrail added the [Pri] BLOCKER label Jan 31, 2018

dereksmart previously requested changes Jan 31, 2018

View reviewed changes

gravityrail added 2 commits January 31, 2018 13:55

Update gridicons and reposition close button on dialog

4f125f2

Better spacing

8e13486

gravityrail added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Design Review Design has been added. Needs a review! labels Jan 31, 2018

Only import the one gridicon we need

1df97fa

Revert unnecessary changes to gridicons

3542c33

gravityrail and others added 3 commits January 31, 2018 14:53

Fix JS error due to unbound handler

68d0794

Fix event binding and restrict to ESC code

3239f7d

Fix eslint error

e4cd4a6

joanrho added [Status] Design Review Complete and removed [Status] Needs Design Review Design has been added. Needs a review! labels Jan 31, 2018

joanrho mentioned this pull request Jan 31, 2018

Admin Page: Fix bar graph appearing over full-screen dialog #8701

Open

dereksmart approved these changes Feb 1, 2018

View reviewed changes

Removed bind calls from property assignments.

91a2052

zinigor approved these changes Feb 1, 2018

View reviewed changes

zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 1, 2018

zinigor merged commit 4a3ad0d into master Feb 1, 2018

matticbot added the [Status] Needs Changelog label Feb 1, 2018

zinigor removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 1, 2018

zinigor deleted the fix/better-dialog-close-ux branch February 1, 2018 19:42

jeherve removed the [Status] Needs Changelog label Feb 1, 2018

jeherve mentioned this pull request Feb 5, 2018

Admin page: confusing how to close Warm Welcome splash without taking suggested actions #8753

Closed

This was referenced Feb 23, 2018

Jumpstart: Can't dismiss jumpstart modal on mobile phones #8744

Closed

Admin Page: cannot disconnect Jetpack from WordPress.com on mobile device #8726

Closed

Jumpstart: Dismiss button could be made more obvious #7666

Closed

gravityrail mentioned this pull request Mar 1, 2018

Position Jumpstart dialog close button same as upgrade dialog close button #8961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable ESC and clicking background to close full screen dialog #8697

Enable ESC and clicking background to close full screen dialog #8697

gravityrail commented Jan 31, 2018 •

edited

Loading

dereksmart Jan 31, 2018

gravityrail Jan 31, 2018

dereksmart Jan 31, 2018

gravityrail Jan 31, 2018

dereksmart commented Jan 31, 2018

dereksmart commented Jan 31, 2018

dereksmart commented Jan 31, 2018

joanrho commented Jan 31, 2018 •

edited

Loading

gravityrail commented Jan 31, 2018

joanrho commented Jan 31, 2018

dereksmart left a comment

zinigor left a comment

zinigor commented Feb 1, 2018

Enable ESC and clicking background to close full screen dialog #8697

Enable ESC and clicking background to close full screen dialog #8697

Conversation

gravityrail commented Jan 31, 2018 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

dereksmart Jan 31, 2018

Choose a reason for hiding this comment

gravityrail Jan 31, 2018

Choose a reason for hiding this comment

dereksmart Jan 31, 2018

Choose a reason for hiding this comment

gravityrail Jan 31, 2018

Choose a reason for hiding this comment

dereksmart commented Jan 31, 2018

dereksmart commented Jan 31, 2018

dereksmart commented Jan 31, 2018

joanrho commented Jan 31, 2018 • edited Loading

gravityrail commented Jan 31, 2018

joanrho commented Jan 31, 2018

dereksmart left a comment

Choose a reason for hiding this comment

zinigor left a comment

Choose a reason for hiding this comment

zinigor commented Feb 1, 2018

gravityrail commented Jan 31, 2018 •

edited

Loading

joanrho commented Jan 31, 2018 •

edited

Loading