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(sidenav): update docs and add examples #7775

Merged
merged 5 commits into from
Nov 21, 2017

Conversation

mmalerba
Copy link
Contributor

No description provided.

@mmalerba mmalerba requested a review from amcdnl as a code owner October 13, 2017 17:39
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 13, 2017
@mmalerba
Copy link
Contributor Author

fixes #7159


<!-- example(sidenav-position) -->

The following are examples of valid sidenav layouts:
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the result of each valid example? Why are the invalid examples invalid?

e.g.

<!-- Creates a layout with a left-positioned sidenav. -->
<mat-sidenav-container>
  <mat-sidenav>Start</mat-sidenav>
  <mat-sidenav-content>Main</mat-sidenav-content>
</mat-sidenav-container>
<!-- Container has two sidenavs but both default to the same side. -->
<mat-sidenav-container>
  <mat-sidenav>Start</mat-sidenav>
  <mat-sidenav position="start">Start 2</mat-sidenav>
</mat-sidenav-container>


<!-- example(sidenav-overview) -->

The sidenav and its associated content live inside of an `<mat-sidenav-container>`:
There are also drawer components, `<mat-drawer-container>`, `<mat-drawer-content>`, and
Copy link
Contributor

Choose a reason for hiding this comment

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

For easy skimming - start this like the previous paragraph.

The drawer component is designed to add side content to a small section of your app.

The side content should be wrapped in a `<mat-sidenav>` element. The `position` property can be used
to specify which end of the main content to place the side content on. `position` can be either
`start` or `end` which places the side content on the left or right respectively in left-to-right
languages. If the `position` is not set, the `start` end will be assumed. A
Copy link
Contributor

Choose a reason for hiding this comment

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

If position is not set, its default value of start will be assumed.


### Setting the sidenav's size

The `<mat-sidenav>` and `<mat-drawer>` will, by default, fit the size of its content. The width can
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to mention that it will fit the size of its content just once - I've had issues where the sidenav fills in with items, then gets a scrollbar and the width changes but the sidenav doesn't update

<mat-sidenav-container></mat-sidenav-container>
```

And these are examples of invalid sidenav layouts:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an overwhelming use of people doing this wrong that made us want to show people how NOT to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They illustrate the restrictions mentioned above, but for people like me who are too lazy to read words that aren't code 😛

A sidenav often needs to behave differently on a mobile vs a desktop display. On a desktop, it may
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be useful to show how to make this happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what the example linked at this point does

Copy link
Contributor

@amcdnl amcdnl left a comment

Choose a reason for hiding this comment

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

Big improvement overall but the lines between sidenav and drawer are a little more blurry though ( maybe its just me tho ).

@mmalerba
Copy link
Contributor Author

comments addressed, PTAL

describes your sidenav, `role="region"` is recommended.

Similarly, the `<mat-sidenav-contnet>` should be given a role based on what it contains. If it
Copy link

Choose a reason for hiding this comment

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

There is a typo in<mat-sidenav-contnet> with regards to 'contnet'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jelbourn
Copy link
Member

@amcdnl @andrewseguin any further comments?

@mmalerba mmalerba added the P2 The issue is important to a large percentage of users, with a workaround label Nov 8, 2017
@mmalerba
Copy link
Contributor Author

rebased, @andrewseguin PTAL

@andrewseguin
Copy link
Contributor

LGTM

@andrewseguin andrewseguin added action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Nov 17, 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 9, 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 P2 The issue is important to a large percentage of users, with a workaround
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants