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] Make composable #773

Closed
quanglam2807 opened this issue Jun 7, 2015 · 41 comments
Closed

[AppBar] Make composable #773

quanglam2807 opened this issue Jun 7, 2015 · 41 comments
Labels
component: app bar This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@quanglam2807
Copy link
Contributor

quanglam2807 commented Jun 7, 2015

The AppBar is too limited with only a few options. Google implements their web apps with full-size menu (and auto collapse to side menu + hamburger in mobile), search box,... I suggest that we should move to prop.children instead.

Something like this

menuItems = [
  { route: 'get-started', text: 'Get Started' },
  { route: 'customization', text: 'Customization' },
  { route: 'components', text: 'Components' },
  { type: MenuItem.Types.SUBHEADER, text: 'Resources' },
  { 
     type: MenuItem.Types.LINK, 
     payload: 'https://github.com/callemall/material-ui', 
     text: 'GitHub' 
  },
  { 
     text: 'Disabled', 
     disabled: true 
  },
  { 
     type: MenuItem.Types.LINK, 
     payload: 'https://www.google.com', 
     text: 'Disabled Link', 
     disabled: true 
  },
];

<AppBar>
  // Extends LeftNav with full-size menu on big screen
  <AppBarMenu menuItems={menuItems} />
  // Auto collapse to search icon on mobile
  <SearchBox handler={searchHandler} />
</AppBar>

What is your idea? I will work on this as soon as we all agree.

@quanglam2807 quanglam2807 changed the title AppBar is too limited AppBar is too limited and suggestions Jun 7, 2015
@hai-cea hai-cea changed the title AppBar is too limited and suggestions [AppBar] Make composable Jun 19, 2015
@hai-cea
Copy link
Member

hai-cea commented Jun 19, 2015

Related to #27

@slara
Copy link

slara commented Jun 23, 2015

👍

This was referenced Jun 25, 2015
@hai-cea hai-cea added this to the Composable Components milestone Jul 13, 2015
@e-monson
Copy link

👍 I really really need this.

@shaurya947
Copy link
Contributor

I've decided to work a new appbar component that addresses this milestone. I'll be following the Material design guidelines for appbar mostly. I have always tried to address issues filed with the [AppBar] prefix. I wanted to share some of my initial ideas here and see what you guys think:

  • For most props, if they're not specified, don't show them (rather than having a separate showIcon.. flag, or displaying something by default)
  • Almost every prop is a node (navIcon, title, menuIcon), and one can pass an array of nodes for the action icons. This also allows the title to be a custom component that may / may not have a filter icon with it, or be a search component etc.
  • Material Design guidelines specify that all icons in the appbar should be the same color. This calls for a new key for appBar inside theme-manager called iconColor. The title may have a different color so we can introduce another key for that.

What do you guys think? @quanglam2807 @oliviertassinari @hai-cea @slara @e-monson

So far everything that I've described is a prop. Speaking of composability, I assume we mean something with children. What use case am I missing? The one with tabs? That feels too specific, doesn't it?

@hai-cea
Copy link
Member

hai-cea commented Sep 30, 2015

@shaurya947 Sounds good to me - children should just be rendered inside the container element to provide some customization flexibility.

@oliviertassinari
Copy link
Member

Regarding composability my approach would be to have multiple AppBar... components. At least one for the container, one for elements at the left and the right. Would be close to what IconMenu is or the Toolbar or the Table. I like the idea of @quanglam2807.

@shaurya947 I feel link your approach is close to what the ItemList is right now. We have seen the limitation of the long list of props of the ListItem. It's more restrictive, gives you less freedoom.
So, I don't think that we need an new AppBar.

@quanglam2807
Copy link
Contributor Author

@oliviertassinari 👍 Thank you for your support!!! 💯
@shaurya947 For the appbar, actually, I've worked on it but I had to delay 'cause I was quite busy. You can check it at https://github.com/quanglam2807/material-ui/tree/app-bar
My approach was similar to Toolbar: when you put a component in Toolbar or AppBar, it will have special style elements. With this approach, with a few customizations, many components like Tabs or DropDownMenu will work perfectly with AppBar.

Also, there will be also components which are specifically designed for AppBar, e.g AppBarTitle, SearchBox, etc.

To work on this idea, I simply modified Toolbar which AppBar's styles. It worked pretty well and what we should aim first is to make the main components work. Then, later step is to add new components, such as SearchBox or better integration between LeftNav and Appbar.

@shaurya947
Copy link
Contributor

I agree guys, instead of using props, it would be nicer to have sub-components for each section of the appBar.

I will work out an initial prototype soon and share it with y'all.

@shaurya947
Copy link
Contributor

More on this:

If you think about it, AppBar really has 4 sections (from left to right):

  • navIcon
  • container
  • actionsIcons
  • menuIcon

If we're creating sub-components for each of these sections, I see two approaches:

  • create a new sub-component for each of the sections above OR
  • create a single generic sub-component and have a type prop to specify the section of the appbar

I like the latter approach more. So if you imagine, you would be able to use the AppBar like so:

<AppBar>
    <AppBarSubComponent type="navIcon" action={some_function()}>
        //put whatever here, style it as you like, simply rendered
    </AppBarSubComponent>

    <AppBarSubComponent type="container">
        //all code here is yours, again simply rendered
    </AppBarSubComponent>

    <AppBarSubComponent type="actionIcons">
        //expects one child for each action icon
        //can use IconButtons or whatever
        //user defines action (pass it as prop)
        //simply rendered from left to right in order specified
    </AppBarSubComponent>

    <AppBarSubComponent type="menuIcon" action={some_other_function()}>
        //put whatever here, style it as you like, simply rendered
    </AppBarSubComponent>

</AppBar>

Feedback, guys? @quanglam2807 @oliviertassinari @hai-cea @slara @e-monson

@quanglam2807
Copy link
Contributor Author

Nice works, @shaurya947. I think having a generic sub-component is the best choice as it's in parallel with Toolbar component. But do you think it's better if we only have 3 types: left, center and right?

@shaurya947
Copy link
Contributor

@quanglam2807:

  • navIcon --> left
  • container --> center
  • actionIcons + menuIcon --> right

Only reason I would prefer 4 types (2 sub-types for the right side) is so that we can handle the layout of actions icons and the menu icon internally (as per Material Design guidelines) without the user having to worry about it.

Also, going back to your first comment on this issue, we could take this even further like so:

<AppBarGroup>
    <AppBar layout = "mobile">
        <AppBarSubComponent type="navIcon" action={some_function()}>
            //put whatever here, style it as you like, simply rendered
        </AppBarSubComponent>

        <AppBarSubComponent type="container">
            //all code here is yours, again simply rendered
        </AppBarSubComponent>

        <AppBarSubComponent type="actionIcons">
            //expects one child for each action icon
            //can use IconButtons or whatever
            //user defines action (pass it as prop)
            //simply rendered from left to right in order of specification
        </AppBarSubComponent>

        <AppBarSubComponent type="menuIcon" action={some_other_function()}>
            //put whatever here, style it as you like, simply rendered
        </AppBarSubComponent>

    </AppBar>

    <AppBar layout = "tablet">
        <AppBarSubComponent type="navIcon" action={some_function()}>
            //put whatever here, style it as you like, simply rendered
        </AppBarSubComponent>

        <AppBarSubComponent type="container">
            //all code here is yours, again simply rendered
        </AppBarSubComponent>

        <AppBarSubComponent type="actionIcons">
            //expects one child for each action icon
            //can use IconButtons or whatever
            //user defines action (pass it as prop)
            //simply rendered from left to right in order of specification
        </AppBarSubComponent>

        <AppBarSubComponent type="menuIcon" action={some_other_function()}>
            //put whatever here, style it as you like, simply rendered
        </AppBarSubComponent>

    </AppBar>

    ...

</AppBarGroup>

Thoughts?

@oliviertassinari
Copy link
Member

But do you think it's better if we only have 3 types: left, center and right?

Agree. From my point of view, the main interest of the AppBar component is to render existing component like Paper, Icon, IconMenu, into something meaningfull. It's almost like a Layout.

@shaurya947
Copy link
Contributor

Here's some skeletal code:

const AppBar = React.createClass({

    //getInitialState and context stuff goes here

    render () {
        return (
      <Paper
        rounded={false}
        className={props.className}
        style={this.mergeAndPrefix(styles.root, props.style)}
        zDepth={props.zDepth}>
          {this._getNavIcon()}
          {this._getContainer()}
          {this._getActionsIcons()}
          {this._getMenuIcon()}
      </Paper>
    );
    },

    _getNavIcon () {
        if((this.props.children[0]).props.type === "navIcon")
        {
            //return something
        }
    },

    _getContainer () {
        let pos = -1;
        if(
            ((this.props.children[0]).props.type === "container" && pos = 0) ||
            ((this.props.children[1]).props.type === "container" && pos = 1)
            )
        {
            //return something
        }
    },

    _getActionsIcons () {
        let pos = -1;
        if(
            ((this.props.children[0]).props.type === "actionIcons" && pos = 0) ||
            ((this.props.children[1]).props.type === "actionIcons" && pos = 1) ||
            ((this.props.children[2]).props.type === "actionIcons" && pos = 2)
            )
        {
            //return something
        }
    },

    _getMenuIcon () {
        let pos = -1;
        if(
            ((this.props.children[0]).props.type === "menuIcon" && pos = 0) ||
            ((this.props.children[1]).props.type === "menuIcon" && pos = 1) ||
            ((this.props.children[2]).props.type === "menuIcon" && pos = 2) ||
            ((this.props.children[3]).props.type === "menuIcon" && pos = 3)
            )
        {
            //return something
        }
    },

});

I'm using pos in the functions to determine the index of the appropriate node from the if condition (without having to iterate through the children). This restricts a specific order to specify the children components: navIcon -> container -> actionIcons -> menuIcon -- this makes sense logical sense to me. Of course, any child component may be overridden.

Keeping you guys in the loop here for some incremental feedback :)

@quanglam2807
Copy link
Contributor Author

@shaurya947 Nicely done! I prefer 3 types but if you think 4 types fit Material Design better, it's perfectly fine for me 💃

@alitaheri alitaheri added the new feature New feature or request label Dec 8, 2015
@leesei
Copy link
Contributor

leesei commented Dec 21, 2015

👍 to this feature.
Looking forward to the PR.

@transitive-bullshit
Copy link

Would also like to see AppBar use an approach similar to Toolbar/ToolbarGroup/etc

@Domiii
Copy link

Domiii commented Oct 13, 2016

Any updates?

Tabs not displaying right in the AppBar is still an issue. You rejected this PR eight months ago. Is there a better solution by now?

This is also being discussed on StackOverflow.

@oliviertassinari
Copy link
Member

@Domiii Nop, that's something we should try to address in the next branch.

@mtrpcic
Copy link

mtrpcic commented Oct 13, 2016

THere's been a lot of activity and discussion here, and I'm eager to find an AppBar implementation that is a bit more composabe. The most recent comment is that we wanted the comosable AppBar in 0.16.0 (on May 20 by newoga), but I haven't seen any mention, and the documentation still shows the old style. Any news here?

@mtrpcic
Copy link

mtrpcic commented Oct 13, 2016

Does anyone have an alternative to the AppBar? I want to have a search field in my main app header, which I'm currently using an AppBar for, and they don't quite play nicely together.

@andrewluchen
Copy link

Any way to create elements in the center of the AppBar? Here, the children of AppBar get located after the right icon, which means "right icon" isn't actually right-most.

https://github.com/callemall/material-ui/blob/master/src/AppBar/AppBar.js#L323

@tejans24
Copy link

tejans24 commented Nov 4, 2016

So, is this being implemented? I have been using a hack and css tweaks to make the links show up properly. This should be part of the core material-ui lib.

@slavab89
Copy link
Contributor

slavab89 commented Nov 5, 2016

@tejans24 I believe that it is. Check #4767 . It seems that the code is already in the next branch.
It seems that it now only have children so you can put anything in there.

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

Hi guys, is this solved? I have tested, but I have this problem too... Thanks!
screen shot 2016-12-28 at 20 51 02

@Luis-Palacios
Copy link

Luis-Palacios commented Jan 6, 2017

Anyone can share example of what they have accomplished so far using css tweaks? i am trying to get some inspiration to try and custom the app bar

@manasa884
Copy link

screen shot 2017-11-14 at 7 28 18 pm
I've tried looking for the fix but I'm also having the same issue. Can someone point me in the right direction?

@sfandrew
Copy link

Anyone with leads on how to add a search input to the AppBar?

@freb
Copy link

freb commented Jan 25, 2018

@sfandrew, the website for the new version has an example (https://material-ui.com/):

image

You can find the source here:

https://github.com/mui/material-ui/blob/v1-beta/docs/src/modules/components/AppFrame.js#L199

@sfandrew
Copy link

@freb I don't see the AppSearch or AppFrame component installed in my node_modules/Material-UI dir. I have the latest version installed. Am I missing something?

@freb
Copy link

freb commented Jan 29, 2018

They aren't modules included with material-ui, so you won't find them in the node_modules folder. They are only visible if you've cloned the repo. They are components that were created for use with the documentation site (https://material-ui.com/). All of the code for the documentation site is found under the docs folder in this repo. But you can use the code there as an example of how to implement your components to add a search input to the App Bar. The AppBar component from the @next branch renders any children provided to it as you would expect, which let's you just pass in a <TextField> for <input> if you want.

@sfandrew
Copy link

Thank you very much @freb!

@oliviertassinari
Copy link
Member

I believe this concern has already been solved with v1. The AppBar component is really simple now. It was breakdown into smaller parts, e.g. the Toolbar. It's not perfect, but we haven't seen any problem with the approach over the last 6 months.

mnajdova pushed a commit to mnajdova/material-ui that referenced this issue Nov 10, 2020
mnajdova pushed a commit to mnajdova/material-ui that referenced this issue Nov 10, 2020
…close-inline

[mui#773] Fix firing onChange with default date if null passed to Inline…
mnajdova pushed a commit to mnajdova/material-ui that referenced this issue Nov 10, 2020
…#904)

* Fix displaying of long dates in DateTimePicker

* [mui#900[ Revert mui#773
mnajdova pushed a commit to mnajdova/material-ui that referenced this issue Nov 10, 2020
* Fix displaying of long dates in DateTimePicker

* [mui#900[ Revert mui#773

* Rename selected and disabled classes to remove warning
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! new feature New feature or request
Projects
None yet
Development

No branches or pull requests