feat(sidenav): track sidenav visibility and enter / leave backdrop if needed #6486

devversion · 2015-12-28T23:00:58Z

Revert scope value if sidenav is shown.
Throttle Resize Events

EladBezalel · 2015-12-31T17:08:12Z

src/components/sidenav/sidenav.js

+    function revalidateVisibility() {
+      var lastValue = scope.isOpen;
+      if (isHidden(element, true, true) && scope.isOpen == true) scope.isOpen = false;
+      else if (!isHidden(element, true, true) && scope.isOpen == false) scope.isOpen = true;


Brackets, scope.isOpen == true is the same as scope.isOpen and ternary operator

devversion · 2016-01-01T21:34:29Z

@EladBezalel - Completed all your suggestions, thanks for reviewing!

EladBezalel · 2016-01-02T10:24:38Z

src/components/sidenav/sidenav.js

@@ -229,6 +229,9 @@ function SidenavDirective($mdMedia, $mdUtil, $mdConstant, $mdTheming, $animate,
    var lastParentOverFlow;
    var triggeringElement = null;
    var promise = $q.when(true);
+    var skipSidenav = false;
+    var skipNextUpdate = false;
+    var $window = angular.element(window);


You can use DI to get $window

Will be better, yes!

devversion · 2016-01-08T08:57:33Z

@EladBezalel - Fixed the suggestions + Improved isHidden Method (more clear and less code) - Can you verify the new version?

EladBezalel · 2016-01-09T09:42:49Z

src/components/sidenav/sidenav.js

+     */
+    function isHidden(computedStyle, offsetParent) {
+      if (computedStyle && window.getComputedStyle(element[0]).display === 'none') return true;
+      if (offsetParent && element[0].offsetParent == null) return true;


why not doing

if (computedStyle && window.getComputedStyle(element[0]).display === 'none' || offsetParent && !element[0].offsetParent) { return true; }

Yes that's also possible, but I thought we should have a clear variant, because this method is really important and needs to be easy to understand, but if you want I can simplify the code?

EladBezalel · 2016-01-09T10:06:22Z

src/components/sidenav/sidenav.js

+        scope.isOpen = false;
+      } else if (!isHidden(true, true) && !scope.isOpen) {
+        scope.isOpen = true;
+      }


Isn't that equals to:

scope.isOpen = !(isHidden(true, true) && scope.isOpen);

No that works not, because I noticed that we only should update if that if statements are true, and we should not set the scope always

Sorry i couldn't understand, are we watching for isOpen changes?

Sorry for the late answer. Yes updateIsOpen (the actual method which triggers open and close animations) is called by watching isOpen.

EladBezalel · 2016-01-24T22:53:09Z

src/components/sidenav/sidenav.js

+     * @param computedStyle Check Computed Style
+     * @param offsetParent Check Offset Parent
+     */
+    function isHidden(computedStyle, offsetParent) {


Is this function is generic enough to be on the util service?

Yes, definitely, should I add it?

Done 👍 Added to Util + added tests to validate the functionality.

EladBezalel · 2016-01-26T07:21:15Z

src/components/sidenav/sidenav.js

-                }
-              });
+      var actions = [isOpen ? $animate.enter(backdrop, parent) : $animate.leave(backdrop)];
+      if (!skipSidenav) actions.push(isOpen ? $animate.removeClass(element, 'md-closed') : $animate.addClass(element, 'md-closed'));


Please break this line

Done ✋ I should definitely use that conventions (how I call it) in my next PR's so we would save much time 😄 - Already done in my newer PR's

EladBezalel · 2016-02-04T19:45:03Z

src/components/sidenav/sidenav.js

+          scope.isOpen = !!oldValue;
+          return;
+        }
+        element.toggleClass('md-closed', wasClosed);


Can't you reuse that?

element.removeClass('md-closed'); if ($mdUtil.isHidden(element, true, false) && !skipSidenav) { skipNextUpdate = true; scope.isOpen = !!oldValue; } element.toggleClass('md-closed', wasClosed);

No, won't work, because I need to revert the class before the scope watcher will be triggered.

element.removeClass('md-closed'); var isHidden = $mdUtil.isHidden(element, true, false); element.toggleClass('md-closed', wasClosed); if (isHidden && !skipSidenav) { skipNextUpdate = true; scope.isOpen = !!oldValue; }

?

This still needs the return, but yes this will work, because we check before removing the class 👍

-> Updated

EladBezalel · 2016-02-06T16:11:19Z

@ThomasBurleson please review

devversion · 2016-05-27T10:24:48Z

@EladBezalel I think this PR is not really a high priority, not much developers are complaining about it. What do you think? Should we remove it from the deprecation progress?

ThomasBurleson · 2016-06-02T13:28:02Z

This issue is closed as part of our ‘Surge Focus on Material 2' efforts.
For details, see our forum posting @ http://bit.ly/1UhZyWs.

ThomasBurleson assigned EladBezalel Dec 29, 2015

EladBezalel reviewed Dec 31, 2015
View reviewed changes

EladBezalel added the needs: work label Dec 31, 2015

devversion force-pushed the feat/sidenav-track-visibility branch 2 times, most recently from b89b094 to 950216f Compare January 1, 2016 21:33

EladBezalel reviewed Jan 2, 2016
View reviewed changes

devversion force-pushed the feat/sidenav-track-visibility branch 3 times, most recently from fcfff73 to b2d0548 Compare January 5, 2016 17:22

EladBezalel reviewed Jan 9, 2016
View reviewed changes

devversion force-pushed the feat/sidenav-track-visibility branch from b2d0548 to 9fd9ed6 Compare January 9, 2016 10:01

EladBezalel reviewed Jan 9, 2016
View reviewed changes

devversion force-pushed the feat/sidenav-track-visibility branch 2 times, most recently from 6e426c1 to e8817ee Compare January 9, 2016 10:24

EladBezalel reviewed Jan 24, 2016
View reviewed changes

devversion force-pushed the feat/sidenav-track-visibility branch from e8817ee to 8c2c521 Compare January 25, 2016 19:20

EladBezalel reviewed Jan 26, 2016
View reviewed changes

devversion force-pushed the feat/sidenav-track-visibility branch from 8c2c521 to 94c0d6f Compare January 26, 2016 12:15

ThomasBurleson modified the milestone: 1.0.5 Feb 4, 2016

EladBezalel reviewed Feb 4, 2016
View reviewed changes

ThomasBurleson modified the milestones: 1.0.5, 1.0.6 Feb 4, 2016

EladBezalel added needs: review This PR is waiting on review from the team and removed needs: work labels Feb 6, 2016

EladBezalel assigned ThomasBurleson and unassigned EladBezalel Feb 6, 2016

devversion added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Mar 21, 2016

ThomasBurleson modified the milestones: 1.0.6, 1.0.8 Apr 4, 2016

ThomasBurleson modified the milestones: 1.0.8, Backlog Apr 21, 2016

ThomasBurleson modified the milestones: - Backlog, Deprecated May 26, 2016

ThomasBurleson closed this Jun 2, 2016

devversion deleted the feat/sidenav-track-visibility branch June 2, 2016 10:15

Splaktar mentioned this pull request Jul 30, 2018

sidenav: does not automatically handle entering hidden state on resize #4595

Open

Splaktar changed the title ~~feat(sidenav): track sidenav vissibilty and enter / leave backdrop if needed~~ feat(sidenav): track sidenav visibility and enter / leave backdrop if needed Jul 30, 2018

Splaktar removed needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved needs: review This PR is waiting on review from the team labels Jul 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sidenav): track sidenav visibility and enter / leave backdrop if needed #6486

feat(sidenav): track sidenav visibility and enter / leave backdrop if needed #6486

devversion commented Dec 28, 2015

EladBezalel Dec 31, 2015

devversion commented Jan 1, 2016

EladBezalel Jan 2, 2016

devversion Jan 2, 2016

devversion commented Jan 8, 2016

EladBezalel Jan 9, 2016

devversion Jan 9, 2016

EladBezalel Jan 9, 2016

devversion Jan 9, 2016

EladBezalel Jan 12, 2016

devversion Jan 24, 2016

EladBezalel Jan 24, 2016

devversion Jan 25, 2016

EladBezalel Jan 25, 2016

devversion Jan 25, 2016

EladBezalel Jan 26, 2016

devversion Jan 26, 2016

EladBezalel Feb 4, 2016

devversion Feb 4, 2016

EladBezalel Feb 4, 2016

devversion Feb 4, 2016

EladBezalel commented Feb 6, 2016

devversion commented May 27, 2016

ThomasBurleson commented Jun 2, 2016

feat(sidenav): track sidenav visibility and enter / leave backdrop if needed #6486

feat(sidenav): track sidenav visibility and enter / leave backdrop if needed #6486

Conversation

devversion commented Dec 28, 2015

Choose a reason for hiding this comment

devversion commented Jan 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion commented Jan 8, 2016

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

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

EladBezalel commented Feb 6, 2016

devversion commented May 27, 2016

ThomasBurleson commented Jun 2, 2016