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] document use of theme.mixins.toolbar & <Toolbar /> when using Appbar variant fixed #17878

Merged
merged 3 commits into from
Oct 15, 2019

Conversation

adeelibr
Copy link
Contributor

This PR is related to #16844

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 14, 2019

No bundle size changes comparing a355648...832493a

Generated by 🚫 dangerJS against 832493a

@adeelibr
Copy link
Contributor Author

@oliviertassinari can you kindly point me in the right direction as to why ci/codesandbox is failing for me, is there something I am missing here?

@mbrookes
Copy link
Member

is there something I am missing here?

Looks like the newly added ci/codesandbox is broken. @eps1lon?

@eps1lon
Copy link
Member

eps1lon commented Oct 15, 2019

is there something I am missing here?

Looks like the newly added ci/codesandbox is broken. @eps1lon?

You can ignore the check for now. It runs without being configured which is a bit unfortunate.

Edit: Rebasing it will make it green though it isn't a requirement at the moment because it's experimental.

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Oct 15, 2019
@eps1lon eps1lon changed the title [Appbar] document use of theme.mixins.toolbar & <Toolbar /> when using Appbar variant fixed [docs] document use of theme.mixins.toolbar & <Toolbar /> when using Appbar variant fixed Oct 15, 2019
@eps1lon eps1lon merged commit 032f7e9 into mui:master Oct 15, 2019
@eps1lon
Copy link
Member

eps1lon commented Oct 15, 2019

@adeelibr Thanks! I'd imagine this is a pretty common footgun (only because I point it at myself all the time)!

@adeelibr adeelibr deleted the document-theme_mixins_toolbar-in-appbar branch October 15, 2019 09:20
## Placement

When using Appbar `variant="fixed"` you need to have extra space for the content to show below
& not under. There are 2 ways to do it. Either use `theme.mixins.toolbar` like;
Copy link
Member

Choose a reason for hiding this comment

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

Do we tend to use & in the docs @mbrookes ?

## Placement

When using Appbar `variant="fixed"` you need to have extra space for the content to show below
& not under. There are 2 ways to do it. Either use `theme.mixins.toolbar` like;
Copy link
Member

Choose a reason for hiding this comment

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

That semicolon should be a colon right?

const useStyles = makeStyles(theme => ({
offset: {
...theme.mixins.toolbar,
flexGrow: 1
Copy link
Member

Choose a reason for hiding this comment

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

Do we need flexGrow in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no flexGrow is not needed, I'll update the PR

```

Or you can append `<Toolbar />` component after `<Appbar />` like shown in the example
below. To prevent content from hiding under Appbar.
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of adding an plain example (to remove and source of doubt and to avoid having to dig in the following demo)?

}
}))

const App = () => {
Copy link
Member

Choose a reason for hiding this comment

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

function App() {?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants