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

[AppBar] Add AppBarContainer component providing padding and layout automatically #4014

Closed
owencm opened this issue Apr 16, 2016 · 7 comments
Labels
component: app bar This is the name of the generic UI component, not the React module! discussion

Comments

@owencm
Copy link
Contributor

owencm commented Apr 16, 2016

Let me know if this has been previously debated, I'd love to read the discussion!

Today I think creating a simple layout with an AppBar at the top of the page is unnecessarily complex, as developers need to manually set the AppBar to fixed position and add padding to whatever is under it with the same height as it will be (which turns out to be spacing.desktopKeylineIncrement).

Has it been discussed previously whether we should instead architect more around a composition model, so for example we could have an <AppBarContainer> (name subject to bikeshedding) component with which you would write:

<AppBarContainer>
  <ThePageContents />
</AppBarContainer>

as shorthand for

<AppBar style={{ position: fixed; top: 0 }} />
<ThePageContents style={{ paddingTop: spacing.desktopKeylineIncrement }} />

I feel like that would make things way easier for developers and would allow for higher-level effects to be implemented like hiding-on-scroll etc.

@owencm
Copy link
Contributor Author

owencm commented Apr 16, 2016

FYI I got thinking about this due to starting implementation on a BottomNavigation component (#3712) and wondering how this will integrate with the Snackbar in terms of layout. A container model definitely seems tidier and the only feasible option if we ever support dynamic height.

@nathanmarks
Copy link
Member

@owencm

For 0.16.0 one of the goals is to solve the issue of styles + all the baggage and inefficiency that comes with replicating simple css functionality such as :hover. This also includes fixing the performance issues created by the current implementation.

This is also an opportunity to plant the seeds of a media query based responsive layout system that would in turn enable what you're saying to be a possibility.

Not everyone agrees that this lib should include such a layout system, but I firmly believe that it would properly allow us to embrace the cross-platform nature of material design. There is an issue somewhere related to this.

@newoga
Copy link
Contributor

newoga commented Apr 16, 2016

@owencm Here is a proof of concept that needs some cleaning up but is what we had in mind for next iteration of AppBar.

http://newoga.github.io/material-ui-scrolling-techniques/
https://github.com/newoga/material-ui-scrolling-techniques

The children of AppBar was going to be used to compose different parts of the AppBar rather than page content. That being said, it still automatically manages padding the dynamic size of the AppBar for you so that you can just provide your <PageContents /> beneath it without having to worry about positioning.

@devdlabs
Copy link

So what is the quick fix for 0.15 ? AppBar default style not usable for most use cases, iconElementRight overflowing to next line, title using h1 tag etc..

@newoga
Copy link
Contributor

newoga commented May 27, 2016

@devdlabs In my projects I use my own <AppBar /> implementation when the existing ones do not suffice. It's a lot simpler and uses standard flex layout to organize the parts of the AppBar. If the only thing you care about is the Toolbar portion of the <AppBar />, then you also may have better luck using material-ui's <Toolbar /> component.

@oliviertassinari oliviertassinari added the component: app bar This is the name of the generic UI component, not the React module! label Dec 18, 2016
@henrylearn2rock
Copy link

Where can I find spacing.desktopKeylineIncremen? I console.log( the this.props.theme ) and spacing is {unit: 8} and desktopKeylineIncremen is nowhere to be found @ 1.0.0-beta.29

Thanks!

@oliviertassinari
Copy link
Member

@henrylearn2rock It's no longer present. We have different layout helpers in theme.mixins.
I'm closing this issue for #4779

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: app bar This is the name of the generic UI component, not the React module! discussion
Projects
None yet
Development

No branches or pull requests

6 participants