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] New and composable #1817

Closed
wants to merge 5 commits into from
Closed

Conversation

shaurya947
Copy link
Contributor

Source code for new AppBar component here: https://github.com/shaurya947/material-ui/blob/new-app-bar/src/app-bar-new.jsx

Usage here: https://github.com/shaurya947/material-ui/blob/new-app-bar/docs/src/app/components/pages/components/app-bar.jsx#L140

Looking forward to your guys' feedback @hai-cea @oliviertassinari @quanglam2807 @cgestes. Once we're good with the code, I'll add new commits to update the documentation.

@shaurya947 shaurya947 added this to the Composable Components milestone Oct 6, 2015
@@ -2,6 +2,7 @@ node_modules
npm-debug.log
build
coverage
muiReadmeMaster
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 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.

Oh my bad. It was this extra thing in my repo that I should get rid of. Thanks for noticing!

@oliviertassinari
Copy link
Member

It's close to this API http://facebook.github.io/react-native/docs/toolbarandroid.html#content. I like it 👍

@shaurya947
Copy link
Contributor Author

That's some good feedback. Glad you like it. Thanks @oliviertassinari!

@shaurya947
Copy link
Contributor Author

So React 0.14.0 is out and that is what Travis is using now.


actionIconsGroup: {
position: 'absolute',
right: 48,
Copy link
Contributor

Choose a reason for hiding this comment

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

Absolute positioning may not work in all cases. Try it out for AppBar in the material-ui docs, I think tabs will be displaced and then you need to tweak top property.
It would be nice to see here a flexbox based positioning.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Yeah, I think that we should you flexbox

Can just use divs for different sections of the appbar. Hopefully, a situation won't arise where the AppBarSubComponent wrapper is needed.
Example use in docs site. Still need to put code for example, and update documentation.
Now user doesn't have to specify default height / width / padding in their JSX.
- Used React.Children utilities to refactor code and reduce nesting of if's
- Changed data-position attribute values to "navIconsGroup", "container", "actionIconsGroup" and "menuIconsGroup"
@oliviertassinari
Copy link
Member

@shaurya947 Is this a work in progress?

@shaurya947
Copy link
Contributor Author

Yeah needs some finishing .. I gotta find time to come back to it.

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 26, 2015
@c0b41
Copy link

c0b41 commented Nov 8, 2015

@shaurya947 any status?

@shaurya947
Copy link
Contributor Author

@ayhankuru sometime next week hopefully!

@vizath
Copy link
Contributor

vizath commented Dec 1, 2015

@pulse00
Copy link

pulse00 commented Feb 19, 2016

Any updates on this?

@oliviertassinari
Copy link
Member

@pulse00
Copy link

pulse00 commented Feb 19, 2016

@oliviertassinari thanks!

@newoga
Copy link
Contributor

newoga commented Feb 27, 2016

I'm going to close this for now in an effort to clean up our PRs. A composable Appbar is definitely planned for 0.15.0 and the draft API can be seen here. We also want make some minor improvements to the Toolbar components that build on some of the ideas introduced in this PR (the AppBar and Toolbar components will be designed to work together and will no longer have overlapping functionality). I think we're getting close, I'll ping everyone in a new PR when we're close to handling it in 0.15.0.

Thanks!

@newoga newoga closed this Feb 27, 2016
@Kielan
Copy link

Kielan commented Jun 10, 2016

Not sure this should be closed until absolute positioning edge cases are fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants