Skip to content

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

Merged
zinigor merged 9 commits intomasterfrom
fix/better-dialog-close-ux
Feb 1, 2018
Merged

Enable ESC and clicking background to close full screen dialog#8697
zinigor merged 9 commits intomasterfrom
fix/better-dialog-close-ux

Conversation

@gravityrail
Copy link
Copy Markdown
Contributor

@gravityrail gravityrail commented Jan 31, 2018 •
edited
Loading

Fixes #7904

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.

close-button-moved

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.

@gravityrail gravityrail requested a review from a team as a code owner January 31, 2018 21:14
@gravityrail gravityrail self-assigned this Jan 31, 2018
@gravityrail gravityrail added 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 gravityrail added this to the 5.8 milestone Jan 31, 2018
dereksmart
dereksmart previously requested changes Jan 31, 2018
View reviewed changes
_inc/client/components/jetpack-dialogue/index.jsx Outdated

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

@dereksmart dereksmart Jan 31, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@gravityrail gravityrail Jan 31, 2018

Choose a reason for hiding this comment

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

Good idea - fixed

Copy link
Copy Markdown
Contributor

@dereksmart dereksmart Jan 31, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@gravityrail gravityrail Jan 31, 2018

Choose a reason for hiding this comment

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

Fixed

@gravityrail gravityrail added [Status] Needs Review This PR is ready for review. [Status] Needs Design Review Design has been added. Needs a review! labels Jan 31, 2018
@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Jan 31, 2018

esc is throwing a console error with the recent changes

@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Jan 31, 2018

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:

screen shot 2018-01-31 at 5 10 36 pm

@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Jan 31, 2018

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

screen shot 2018-01-31 at 5 14 12 pm

@joanrho
Copy link
Copy Markdown
Contributor

joanrho commented Jan 31, 2018 •
edited
Loading

@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:

screen shot 2018-01-31 at 3 31 52 pm

@gravityrail
Copy link
Copy Markdown
Contributor Author

gravityrail commented Jan 31, 2018

@joanrho - icon updated, and screenshot in description

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

@gravityrail gravityrail dismissed dereksmart’s stale review January 31, 2018 22:32

Made some changes

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

joanrho commented Jan 31, 2018

Design review approved. Thanks for the edits, Dan!

@joanrho joanrho mentioned this pull request Jan 31, 2018
Closed
dereksmart
dereksmart approved these changes Feb 1, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

LGTM!

zinigor
zinigor approved these changes Feb 1, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

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

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Feb 1, 2018
@zinigor zinigor merged commit 4a3ad0d into master Feb 1, 2018
@zinigor zinigor removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 1, 2018
@zinigor zinigor deleted the fix/better-dialog-close-ux branch February 1, 2018 19:42
zinigor pushed a commit that referenced this pull request Feb 1, 2018
@gravityrail @zinigor
Enable ESC and clicking background to close full screen dialog (#8697)
36b4d68 
* 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
Copy link
Copy Markdown
Contributor

zinigor commented Feb 1, 2018

Ported to branch-5.8 in 36b4d68.

@jeherve jeherve mentioned this pull request Feb 5, 2018
Closed
This was referenced Feb 23, 2018
Closed
Closed
Closed
@gravityrail gravityrail mentioned this pull request Mar 1, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dereksmart dereksmart dereksmart approved these changes

+1 more reviewer

@zinigor zinigor zinigor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gravityrail gravityrail

Labels

Bug When a feature is broken and / or not performing as intended [Focus] Accessibility Improving usability for all users (a11y) [Pri] BLOCKER [Status] Design Review Complete

Projects

None yet

Milestone

5.8

Development

Successfully merging this pull request may close these issues.

Admin Page: no option to dismiss Warm Welcome

6 participants

@gravityrail @dereksmart @joanrho @zinigor @jeherve @matticbot