Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(drawer): Add additional documentation for disableClose #6750

Merged
merged 5 commits into from
Sep 8, 2017

Conversation

g1shin
Copy link

@g1shin g1shin commented Aug 31, 2017

ESCAPE keydown is exposed as an event to support various cases.

Fixes #6427

@g1shin g1shin requested review from jelbourn, kara and mmalerba August 31, 2017 00:13
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 31, 2017
@mmalerba
Copy link
Contributor

mmalerba commented Sep 6, 2017

@willshowell you mentioned in #6427 that dialog already does something like this? Do you know what the Observable is called? I'd like to keep it consistent across components

@willshowell
Copy link
Contributor

@mmalerba it hasn't yet been merged but it will be a generic keydownEvents(). See #6682 and specifically dialog-ref.ts.

It was @jelbourn's opinion (#3460 (comment)) that the consumer can filter for desired keys, but that may have only been because overlay is very generic.

@g1shin
Copy link
Author

g1shin commented Sep 6, 2017

@mmalerba Would you recommend changing escapeKeydown to keydownEvents then?

@mmalerba
Copy link
Contributor

mmalerba commented Sep 6, 2017

I actually wonder if we even need it for sidenav, it seems like its just a proxy for (keydown) on md-drawer, not something that needs its own @Output(). Instead maybe just update the documentation to show how it can be done:

<md-drawer-container (backdropClick)="customBackdropClickHandler()">
  <md-drawer disableClose (keydown)="customKeydownHandler($event)"></md-drawer>
</md-drawer-container>

@g1shin
Copy link
Author

g1shin commented Sep 6, 2017

Reverted changes made to escapeKeydown and updated the doc instead.

### Sidenav vs. Drawer
The difference between sidenav and drawer is that the sidenav takes up fullscreen whereas drawer is a smaller element
that takes up a subsection of the screen. `md-sidenav` has to be placed inside `md-sidenav-container` and `md-drawer`
has to be placed inside `md-drawer-container`. More functionalities will be added to sidenav in the future.
Copy link
Contributor

Choose a reason for hiding this comment

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

For the last sentence how about:

Currently `md-drawer` and `md-sidenav`support all the same features. However in the future we expect to add different features specific to the different use cases.

@@ -70,3 +70,19 @@ html, body, material-app, md-sidenav-container, .my-content {
### FABs inside sidenav
For a sidenav with a FAB (Floating Action Button) or other floating element, the recommended approach is to place the FAB
outside of the scrollable region and absolutely position it.

### Sidenav vs. Drawer
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put this at the top since its important context that users need to understand the rest of the docs (that all the stuff we're saying about sidenavs also applies to drawer


### Disabling closing of sidenav and drawer
By default, sidenav and drawer can be closed either by clicking on the backdrop or by pressing <kbd>ESCAPE</kbd>.
`disableClose` attribute can be set on `md-drawer` or `md-sidenav` to disable closing by two methods. To enable one
Copy link
Contributor

Choose a reason for hiding this comment

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

The disableClose attribute

"closing by two methods" --> "automatic closing when the backdrop is clicked or the escape key is pressed"

### Disabling closing of sidenav and drawer
By default, sidenav and drawer can be closed either by clicking on the backdrop or by pressing <kbd>ESCAPE</kbd>.
`disableClose` attribute can be set on `md-drawer` or `md-sidenav` to disable closing by two methods. To enable one
of the two methods to allow closing with `disableClose` attribute set, then custom event handlers can be used as below:
Copy link
Contributor

Choose a reason for hiding this comment

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

To add custom logic on backdrop click, add a (backdropClick) listener to the md-sidenav-container. For custom escape key logic, a standard (keydown) listener will suffice.

`disableClose` attribute can be set on `md-drawer` or `md-sidenav` to disable closing by two methods. To enable one
of the two methods to allow closing with `disableClose` attribute set, then custom event handlers can be used as below:
```html
<md-drawer-container (backdropClick)="customBackdropClickHandler()">
Copy link
Contributor

Choose a reason for hiding this comment

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

Since every other example in this doc uses sidenav, lets use sidenav. The new explainer about sidenav vs drawer will let people know they can use either

@g1shin
Copy link
Author

g1shin commented Sep 7, 2017

Comments have been addressed 👍 Ready for review

@@ -18,6 +18,14 @@ A sidenav container may contain one or two `<md-sidenav>` elements. When there a
`<md-sidenav>` elements, each must be placed on a different side of the container.
See the section on positioning below.


### Sidenav vs. Drawer
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't make this a separate section, just part of the section above. It also needs to be worded to introduce the drawer, otherwise us: "The difference between sidenav and drawer..." user: "wait, what's drawer?"

@g1shin
Copy link
Author

g1shin commented Sep 8, 2017

Changes have been made 😃

@@ -18,6 +18,13 @@ A sidenav container may contain one or two `<md-sidenav>` elements. When there a
`<md-sidenav>` elements, each must be placed on a different side of the container.
See the section on positioning below.

`<md-drawer>` is a component that is similar to `<md-sidenav>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought I'll just add some documentation on md-drawer later, for now lets not mention it since its the exact same thing as md-sidenav


### Disabling closing of sidenav and drawer
By default, sidenav and drawer can be closed either by clicking on the backdrop or by pressing <kbd>ESCAPE</kbd>.
The `disableClose` attribute can be set on `md-drawer` or `md-sidenav` to disable automatic closing when the backdrop
Copy link
Contributor

Choose a reason for hiding this comment

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

remove mention of md-drawer

@g1shin
Copy link
Author

g1shin commented Sep 8, 2017

Removed everything related to md-drawer 👍

@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Sep 8, 2017
@mmalerba mmalerba changed the title fix(drawer): Add additional support for disableClose docs(drawer): Add additional documentation for disableClose Sep 8, 2017
By default, sidenav can be closed either by clicking on the backdrop or by pressing <kbd>ESCAPE</kbd>.
The `disableClose` attribute can be set on `md-sidenav` to disable automatic closing when the backdrop
is clicked or <kbd>ESCAPE</kbd> is pressed. To add custom logic on backdrop click, add a `(backdropClick)` listener to
`md-sidenav-container`. For custom <kdb>ESCAPE</kbd> logic, a standard `(keydown)` listener will suffice.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo <kdb> -> <kbd>

@tinayuangao tinayuangao merged commit 4f4c48c into angular:master Sep 8, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disableClose in sidenav needs improvement
6 participants