Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(sidenav): allow to hide side nav depending on the media query #11581

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marosoft
Copy link
Contributor

@marosoft marosoft commented Jan 7, 2019

Current behavior is not correct

Fixes #4595

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

At the moment it is not possible to hide side nav without breaking the view - the overlays stays visible.
Issue Number: #4595

What is the new behavior?

A new attribute allows define when the side nav should hide depending on the media query.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jan 7, 2019
@Splaktar Splaktar self-requested a review January 8, 2019 22:19
@Splaktar Splaktar self-assigned this Jan 8, 2019
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

I need to do some more thinking about the naming of this API, but the current name of is-locked-closed will have to be changed.

In the meantime, I've left some comments and CodePen examples that I've been working with.

$media: function(arg) {
$log.warn("$media is deprecated for is-locked-closed. Use $mdMedia instead.");
return $mdMedia(arg);
},
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we need to add this old deprecated API warning for the new isLockedClosed API.

@@ -244,16 +245,19 @@ function SidenavFocusDirective() {
* @param {expression=} md-is-locked-open When this expression evaluates to true,
* the sidenav 'locks open': it falls into the content's flow instead
* of appearing over it. This overrides the `md-is-open` attribute.
Copy link
Member

Choose a reason for hiding this comment

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

In the MD Spec's Surface Behaviors when this value is true, it maps to Permanent. The default when this is false is Temporary. If you want to implement the Persistant behavior, then you need to allow the value of md-is-locked-open to be toggled like in this CodePen example.

We should link to this part of the spec in the main SideNav docs (above) and mention these behavior names. We also need to have demos of each type of behavior (could be as part of 1 or 2 demos).

@@ -244,16 +245,19 @@ function SidenavFocusDirective() {
* @param {expression=} md-is-locked-open When this expression evaluates to true,
* the sidenav 'locks open': it falls into the content's flow instead
* of appearing over it. This overrides the `md-is-open` attribute.
* @param {expression=} md-is-locked-closed When this expression evaluates to true,
* the sidenav 'locks closed': hides sidenav.
Copy link
Member

Choose a reason for hiding this comment

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

I'm still trying to get a handle on what behavior this API really has and how to name it.

Based on the naming, md-is-locked-closed="true" should make it so that you can't open the sidenav at all, even via toggle(). But in this CodePen you can see that is not the case. This doesn't mean that your behavior is incorrect, it just means that we have the wrong naming of this API here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Splaktar what if I change it to md-is-hidden, is-hidden-when or md-hide-when?
Basically by specifying the condition you determine when the sidenav has to become closed after it was opened.

@Splaktar Splaktar added needs: review This PR is waiting on review from the team in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs P2: required Issues that must be fixed. and removed needs: review This PR is waiting on review from the team labels Jan 8, 2019
@Splaktar Splaktar added this to the 1.1.13 milestone Jan 8, 2019
@Splaktar Splaktar added the g3: reported The issue was reported by an internal or external product team. label Jan 8, 2019
@Splaktar Splaktar modified the milestones: 1.1.13, 1.1.14 Feb 10, 2019
@Splaktar Splaktar modified the milestones: 1.1.14, 1.1.15 Mar 14, 2019
@Splaktar Splaktar modified the milestones: 1.1.15, 1.1.16, 1.1.18, 1.1.19 Mar 29, 2019
@Splaktar Splaktar modified the milestones: 1.1.19, 1.1.21 May 30, 2019
@Splaktar Splaktar modified the milestones: 1.1.21, 1.1.22 Aug 15, 2019
@Splaktar Splaktar modified the milestones: 1.1.22, 1.1.23 Oct 22, 2019
@Splaktar
Copy link
Member

@marosoft let's revisit this one for 1.1.23?

@Splaktar

This comment has been minimized.

@Splaktar Splaktar removed this from the 1.1.23 milestone Jun 19, 2020
@Splaktar Splaktar added this to the 1.2.1 milestone Jun 19, 2020
@Splaktar Splaktar added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 13, 2020
@Splaktar Splaktar modified the milestones: 1.2.1, 1.2.2 Sep 23, 2020
@Splaktar Splaktar modified the milestones: 1.2.2, - Backlog Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ g3: reported The issue was reported by an internal or external product team. in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved P2: required Issues that must be fixed. ui: responsive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sidenav: does not automatically handle entering hidden state on resize
3 participants