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

Conversation

@marosoft
Copy link
Contributor

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

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

@SplaktarSplaktar 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
Contributor

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.

* @param{string=} md-component-id componentId to use with $mdSidenav service.
* @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
Contributor

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).

* 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
Contributor

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
ContributorAuthor

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.

@SplaktarSplaktar 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
@SplaktarSplaktar added this to the 1.1.13 milestone Jan 8, 2019
@SplaktarSplaktar added the g3: reported The issue was reported by an internal or external product team. label Jan 8, 2019
@SplaktarSplaktar modified the milestones: 1.1.13, 1.1.14Feb 10, 2019
@SplaktarSplaktar modified the milestones: 1.1.14, 1.1.15Mar 14, 2019
@SplaktarSplaktar modified the milestones: 1.1.15, 1.1.16, 1.1.18, 1.1.19Mar 29, 2019
@SplaktarSplaktar modified the milestones: 1.1.19, 1.1.21May 30, 2019
@SplaktarSplaktar modified the milestones: 1.1.21, 1.1.22Aug 15, 2019
@SplaktarSplaktar modified the milestones: 1.1.22, 1.1.23Oct 22, 2019
@Splaktar
Copy link
Contributor

@marosoft let's revisit this one for 1.1.23?

@Splaktar

This comment has been minimized.

@SplaktarSplaktar removed this from the 1.1.23 milestone Jun 19, 2020
@SplaktarSplaktar added this to the 1.2.1 milestone Jun 19, 2020
@SplaktarSplaktar 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
@SplaktarSplaktar modified the milestones: 1.2.1, 1.2.2Sep 23, 2020
@SplaktarSplaktar modified the milestones: 1.2.2, - BacklogDec 21, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yesPR author has signed Google's CLA: https://opensource.google.com/docs/cla/g3: reportedThe issue was reported by an internal or external product team.in progressMainly for in progress PRs, but may be used for issues that require multiple PRsneeds: rebaseThis PR needs to be rebased on the latest commits from master and conflicts need to be resolvedP2: requiredIssues 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

@marosoft@Splaktar@googlebot