feat(sidenav): add disable click and escape event #11165

FjVillar · 2018-03-13T14:30:41Z

Add a property to disable the click and Escape button actions to prevent the user
from closing the sidenav but still showing the backdrop.

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number:
#4699

What is the new behavior?

Now it is possible to add a new attribute called md-disable-click-escape-events to prevent the sidenav from closing when clicking on the backdrop or pressing ESC. If you add it to a md-sidenav it will still show the backdrop but the user won't be able to close it with the previous named events and will force the user to close it by some button or action you've created on your sidenav.

Does this PR introduce a breaking change?


[ ] Yes
[x] No

Other information

googlebot · 2018-03-13T14:30:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

FjVillar · 2018-03-13T15:28:21Z

I have tried to fix the CLA, lets see if it works...

googlebot · 2018-03-13T15:28:24Z

CLAs look good, thanks!

FjVillar · 2018-03-13T15:33:16Z

Seems like i'm having the same issue as this other PR #11166, any clue on what could be the problem?

Splaktar · 2018-03-13T19:23:32Z

@FjVillar don't worry about the snapshot failures at this time. There is something wrong with the AngularJS snapshots and the team is investigating. Thank you for your contribution!

Splaktar · 2018-03-13T19:26:12Z

Please add a blank line and then Closes #4699 to the bottom of your commit message.

Splaktar · 2018-03-14T01:48:16Z

I think that Travis might be fixed, but may not work until you force push up your changes after the review. I wouldn't worry about it for now.

FjVillar · 2018-03-14T07:22:42Z

I did update the commit message : )

FjVillar · 2018-03-19T07:40:44Z

I think the Travis issue returned, is already fixed? @Splaktar

Splaktar

This is a good start, thank you! I would like to refine the API, demo, and documentation a bit.

Splaktar · 2018-03-22T08:58:52Z

src/components/sidenav/demoClickOutsideToClose/index.html

+
+        <md-content layout-margin="">
+          <p>
+            This sidenav is still showing the backdrop, but users can't close the sidenav on clicking or pressing 'ESC' button.


Let's change this a little for clarity "This sidenav is showing the backdrop, but users can't close the sidenav by clicking on the backdrop or pressing 'ESC' button."

Splaktar · 2018-03-22T09:03:14Z

src/components/sidenav/demoClickOutsideToClose/index.html

+
+        <div layout="column" layout-align="top center">
+          <p>
+            Developers can also disable the click and 'Escape' button events.<br/>


Let's change this to "Developers can disable closing the sidenav on backdrop clicks and 'Escape' key events."

Splaktar · 2018-03-22T09:03:38Z

src/components/sidenav/demoClickOutsideToClose/index.html

+        <div layout="column" layout-align="top center">
+          <p>
+            Developers can also disable the click and 'Escape' button events.<br/>
+            This will disable the functionality to click outside to close the sidenav.


This second line is probably not needed then.

Splaktar · 2018-03-22T09:06:29Z

src/components/sidenav/demoClickOutsideToClose/index.html

+
+          <div>
+            <md-button ng-click="toggleSidenav()" class="md-raised">
+              Toggle Sidenav


Let's call this Open Sidenav.

Splaktar · 2018-03-22T09:42:33Z

src/components/sidenav/sidenav.js

@@ -231,6 +231,7 @@ function SidenavFocusDirective() {
 *
 * @param {expression=} md-is-open A model bound to whether the sidenav is opened.
 * @param {boolean=} md-disable-backdrop When present in the markup, the sidenav will not show a backdrop.
+ * @param {boolean=} md-disable-click-key-events When present in the markup, the click outside and 'ESC' button will not close the sidenav.


Can we rename this to md-disable-close-events? So it would be possible to close via toggle or open functions, but not via events (backdrop click or escape key).

With a description like: "When present in the markup, clicking the backdrop or pressing the 'ESC' key will not close the sidenav."

Does this make sense? Do you have any ideas for a better API?

Yes, it sounds good to me :).

Splaktar · 2018-03-22T09:44:03Z

src/components/sidenav/demoClickOutsideToClose/script.js

@@ -0,0 +1,11 @@
+angular
+  .module('sidenavDemo3', ['ngMaterial'])


I would like to get rid of this sidenavDemoX pattern and name the module after the demo, i.e. sidenavDisableCloseEvents.

What do you think about a pattern like this customSidenavDemo, basicSidenavDemo, disableCloseEventsSidenavDemo ?

Sounds good!

Splaktar · 2018-03-22T09:45:02Z

src/components/sidenav/demoClickOutsideToClose/index.html

+          </p>
+
+          <div>
+            <md-button ng-click="toggleSidenav()" class="md-raised">


Let's throw a md-primary on here too :)

It this also wanted in the others demo buttons?

You could do it in the custom sidenav demo's "Toggle Sidenav" button, but I would leave the other flat buttons.

Splaktar · 2018-03-22T09:47:31Z

src/components/sidenav/sidenav.js

@@ -302,6 +303,12 @@ function SidenavDirective($mdMedia, $mdUtil, $mdConstant, $mdTheming, $mdInterac
    if (!attr.hasOwnProperty('mdDisableBackdrop')) {
      backdrop = $mdUtil.createBackdrop(scope, "md-sidenav-backdrop md-opaque ng-enter");
    }
+
+    // If md-disable-click-key-events is set on the sidenav we will disable
+    // click and Escape events


"backdrop click and Escape key events"

Splaktar · 2018-03-22T09:49:12Z

Yes, please ignore these TravisCI failures.

FjVillar · 2018-03-26T10:28:38Z

@Splaktar I have done the requested changes. If you see something It could be improved I will be happy to do it.

Splaktar

Thank you for the updates. This is heading in the right direction. I had a few more comments mostly related to consistency.

Splaktar · 2018-03-26T18:57:23Z

src/components/sidenav/demoDisableCloseEvents/index.html

+        <md-content layout-margin="">
+          <p>
+            This sidenav is showing the backdrop, but users can't close the
+             sidenav by clicking on the backdrop or pressing 'ESC' button.


Please use "'Escape' key" instead of "'ESC' button" to be consistent with the other messaging (like on line 29).

Splaktar · 2018-03-26T19:00:07Z

src/components/sidenav/demoDisableCloseEvents/script.js

@@ -0,0 +1,11 @@
+angular
+  .module('disableCloseEventsSidenavDemo', ['ngMaterial'])
+  .controller('AppCtrl', function ($scope, $timeout, $mdSidenav) {


It looks like $timeout isn't used? If so, please remove it.

Splaktar · 2018-03-26T19:01:00Z

src/components/sidenav/sidenav.js

@@ -231,6 +231,7 @@ function SidenavFocusDirective() {
 *
 * @param {expression=} md-is-open A model bound to whether the sidenav is opened.
 * @param {boolean=} md-disable-backdrop When present in the markup, the sidenav will not show a backdrop.
+ * @param {boolean=} md-disable-close-events When present in the markup, clicking the backdrop or pressing the 'ESC' key will not close the sidenav.


We should be consistent on calling it "'Escape'" instead of "'ESC'".

Splaktar · 2018-03-26T19:02:16Z

src/components/sidenav/sidenav.js

@@ -351,8 +358,12 @@ function SidenavDirective($mdMedia, $mdUtil, $mdConstant, $mdTheming, $mdInterac
      var focusEl = $mdUtil.findFocusTarget(element) || $mdUtil.findFocusTarget(element,'[md-sidenav-focus]') || element;
      var parent = element.parent();

-      parent[isOpen ? 'on' : 'off']('keydown', onKeyDown);
-      if (backdrop) backdrop[isOpen ? 'on' : 'off']('click', close);
+      // If the user hasn't set disable close events property we are adding 


Let's add a "the" in there. "If the user hasn't set the disable close events property we are adding"

Splaktar · 2018-03-26T19:04:19Z

src/components/sidenav/sidenav.spec.js

@@ -59,6 +59,40 @@ describe('mdSidenav', function() {
      expect($rootScope.show).toBe(false);
    }));

+    describe('disable click and ESC key events if md-disable-close-events is set to true',


Let's use Escape instead of ESC here too.

FjVillar · 2018-03-27T07:34:46Z

Lets check it now @Splaktar. Thanks for your review!

Splaktar

Almost there! I just had one minor docs comment.

Splaktar · 2018-03-27T08:54:22Z

src/components/sidenav/demoDisableCloseEvents/index.html

+        <md-content layout-margin="">
+          <p>
+            This sidenav is showing the backdrop, but users can't close the
+             sidenav by clicking on the backdrop or pressing 'Escape' key button.


Let's do "pressing the 'Escape' key." This adds "the" and removes "button".

Add a property to disable the click and Escape button actions to prevent the user from closing the sidenav but still showing the backdrop. Closes angular#4699

FjVillar · 2018-03-27T15:14:41Z

Done!

Splaktar

LGTM

Splaktar · 2018-03-27T22:34:54Z

Thanks, looks good! I just re-ran the 3 failing builds (due to 504 gateway timeouts).

Splaktar · 2018-03-27T23:16:06Z

Yay, tests are finally green! Submitted to presubmit. Thank you for hanging in there!

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Mar 13, 2018

googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Mar 13, 2018

Splaktar requested review from devversion and Splaktar March 13, 2018 19:24

Splaktar self-assigned this Mar 13, 2018

Splaktar added needs: review This PR is waiting on review from the team type: enhancement P4: minor Minor issues. May not be fixed without community contributions. labels Mar 13, 2018

Splaktar added this to the 1.1.9 milestone Mar 13, 2018

FjVillar force-pushed the addDisableClickKeyToSidenav branch from 91e8f6b to 1559af9 Compare March 14, 2018 07:20

Splaktar suggested changes Mar 22, 2018

View reviewed changes

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed needs: review This PR is waiting on review from the team labels Mar 22, 2018

FjVillar force-pushed the addDisableClickKeyToSidenav branch 2 times, most recently from 3ba974e to 3f1286a Compare March 26, 2018 10:17

Splaktar reviewed Mar 26, 2018

View reviewed changes

FjVillar force-pushed the addDisableClickKeyToSidenav branch from 3f1286a to 3b31883 Compare March 27, 2018 07:25

Splaktar reviewed Mar 27, 2018

View reviewed changes

FjVillar force-pushed the addDisableClickKeyToSidenav branch from 3b31883 to a64b647 Compare March 27, 2018 10:07

feat(sidenav): add disable click and escape event

632caf7

Add a property to disable the click and Escape button actions to prevent the user from closing the sidenav but still showing the backdrop. Closes angular#4699

FjVillar force-pushed the addDisableClickKeyToSidenav branch from a64b647 to 632caf7 Compare March 27, 2018 10:59

Splaktar approved these changes Mar 27, 2018

View reviewed changes

Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Mar 27, 2018

josephperrott merged commit fae4042 into angular:master Mar 29, 2018

FjVillar deleted the addDisableClickKeyToSidenav branch April 1, 2018 18:30

chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018

feat(sidenav): add disable click and escape event (angular#11165)

f0baecf

Splaktar mentioned this pull request Jun 29, 2018

fix(sidenav): refactor syntax to work around issue in babel-minify #11351

Merged

3 tasks

Splaktar pushed a commit that referenced this pull request Jul 31, 2018

feat(sidenav): add disable click and escape event (#11165)

c60982b

		@@ -0,0 +1,11 @@
		angular
		.module('sidenavDemo3', ['ngMaterial'])

feat(sidenav): add disable click and escape event #11165

feat(sidenav): add disable click and escape event #11165

Conversation

FjVillar commented Mar 13, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Mar 13, 2018

FjVillar commented Mar 13, 2018 • edited Loading

googlebot commented Mar 13, 2018

FjVillar commented Mar 13, 2018 • edited Loading

Splaktar commented Mar 13, 2018

Splaktar commented Mar 13, 2018

Splaktar commented Mar 14, 2018

FjVillar commented Mar 14, 2018

FjVillar commented Mar 19, 2018 • edited Loading

Splaktar 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

FjVillar Mar 26, 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

Choose a reason for hiding this comment

Splaktar commented Mar 22, 2018

FjVillar commented Mar 26, 2018

Splaktar 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

FjVillar commented Mar 27, 2018

Splaktar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FjVillar commented Mar 27, 2018

Splaktar left a comment

Choose a reason for hiding this comment

Splaktar commented Mar 27, 2018

Splaktar commented Mar 27, 2018

FjVillar commented Mar 13, 2018 •

edited

Loading

FjVillar commented Mar 13, 2018 •

edited

Loading

FjVillar commented Mar 19, 2018 •

edited

Loading

FjVillar Mar 26, 2018 •

edited

Loading