[IconMenu] Support MenuItem nested menuItems

[tommrr]

If nested menuItem is used in MenuItem they close instantaneously.

This is an example of the bug. http://imgur.com/g4M32q4

[mbrookes]

Thanks for your PR! 👍

https://github.com/callemall/material-ui/blob/master/CONTRIBUTING.md

[oliviertassinari]

I don't understand this line. That's not supposed to be a number.

    onItemTouchTap: React.PropTypes.func,

[tommrr]

Try that, hopefully I haven't got the wrong object this time.

[mbrookes]

"an Icon Menu" (or "an IconMenu") - I'm still never sure which, despite the fact I wrote all those descriptions. 😄

Icon Menu to refer to the feature, IconMenu to refer to the React component.

[newoga]

I agree, I think whenever we're mentioning another component we should use the component name with back ticks.

`IconMenu`

[tommrr]

Would you like me to change the description? To IconMenu then?

[mbrookes]

I'm wondering if this can be achieved without the extra prop? What is Menu / MenuItem doing for nested menus?

[tommrr]

@mbrookes I'm not using an extra prop, the prop was already there and when you set it to less than 1 then with the original code the menu just disappears, which is useless if you have nested menus as the main menu disappears before you can click on the nested menu. In the project I am working on that uses this library we have a very deep context menu if we have it all in a one level drop down then it easily goes of the bottom of the screen, so using nested menus is a must.

[oliviertassinari]

What about touchTapCloseDelay={0}?
However, that sounds like a hack.

[oliviertassinari]

I'm wondering if this can be achieved without the extra prop?

I agree, I think that we should be checking for a menuItems property on the MenuItem component clicked. If it's not there, we close the menu. child.props.menuItems

[tommrr]

Updated with the feedback. Let me know if anything else needs changing.

[mbrookes]

Could you call the file ExampleNested.jsx please.

[oliviertassinari]

@tomrosier I haven't tested it, but that looks great from a diff point of view 👍.

[oliviertassinari]

What do you think about add a MenuItem without a menuItems property in this example?

[oliviertassinari]

@tomrosier That looks good to me. Could you squash down the commits and have a look at my comment?

[mbrookes]

"an IconMenu"

[oliviertassinari]

That looks good to me 👍.

[alitaheri]

This is great 👍 Thanks a lot 😁

[Dreculah]

I have a very similar requirement, and using the 'children' prop of menuItems almost works for my leftNav menu items.

Using redux, I am passing in an Immutable Map (and ignoring the React warning). As you can see, it does make the indented children. However, it puts them ABOVE the primaryText vs below them.

Is there something I'm doing wrong, or (1) we should have a config to specify children location or (2) children should be below primaryText?

Thanks!

render() {
        let me = this,
            getSubItems = (subItems) => {
                if(!subItems) return null;
                return subItems.map(function(subItem, key) {
                    let subName = subItem.get('name'),
                        subKey  = 'sub' + key;
                    if(EnableConsoleLog) console.log('subName: ', subName);
                    return (<MenuItem
                        key={subKey}
                        primaryText={subName}
                        onTouchTap={me.handleToggle.bind(me)}
                    />)
                });
        },
        menuItems = me.props.menuItems.map(function(menuItem, key) {
            let name     = menuItem.get('name'),
                subItems = menuItem.get('subItems');
                if(EnableConsoleLog) console.log('name: ', name);
                return (<MenuItem
                    key={key}
                    primaryText={name}
                    onTouchTap={me.handleToggle.bind(me)}
                    insetChildren={true}
                    children={getSubItems(subItems)}
                />)
        });
        return (
            <div id="my-page-container">
                <LeftNav
                    ref="leftNav"
                    docked={false}
                    open={me.state.open}
                    children={menuItems}
                />
                <AppBar title='React Reflux Material UI' onLeftIconButtonTouchTap={me.handleToggle.bind(me)}/>
                <div style={styles.container}>
                    I am the app component with mui
                </div>
            </div>
        )
    }

store.dispatch({
    type: 'SET_STATE',
    state: {
        uiState: {
            aKey: 'aValue'
        },
        domainState: {
            menuItems: {
                lastViewed: {
                    name: 'Recent Items',
                    subItems: {
                        salesOrders: {
                            name: 'Pythons'
                        },
                        creditMemos: {
                            name: 'Penguins'
                        }
                    }
                },
                dashboard: {
                    name: 'Dashboard'
                },
                sales: {
                    name: 'Animals',
                    subItems: {
                        customers: {
                            name: 'Puppies'
                        },
                        salesOrders: {
                            name: 'Parakeets'
                        },
                        shipments: {
                            name: 'Pythons'
                        },
                        customerInvoices: {
                            name: 'Parrots'
                        },
                        creditMemos: {
                            name: 'Peacocks'
                        },
                        customerReceipts: {
                            name: 'Penguins'
                        }
                    }
                },
                financials: {
                    name: 'Mordant'
                },
                allPages: {
                    name: 'All Pages'
                }
            }
        }
    }
});

[alitaheri]

@Dreculah Are you using OrderedMap? because Map doesn't guarantee order.

[Dreculah]

@alitaheri This logic is not contingent on sorting, since there will only be subMenu items produced produced and added as children for an object with subItems. I also see the expected console.log messages and as you can see below the DOM is built with the expected nesting (pythons and penguins as subItems of Recent Items), but not rendered as such...

React devTools:

actual DOM:

[alitaheri]

Please try toJS() if that fixes it it's react, if not it's us O.o

[Dreculah]

I switched from a hash map to array so I could use toJS() for map iteration, and I still see exactly the same problem.

Unfortunately, I don't know the material-ui & react source code yet, so I can't spot the problem straight away. It is odd that the React DOM would have the expected structure, but not the browser DOM.

[alitaheri]

Yeah exactly! This is really surprising to me! Can you reproduce this using a very simple array?

[Dreculah]

The array is pretty simple now (please see below).

Maybe its a React bug?

menuItems: [
                {
                    name: 'Recent Items',
                    subItems: [
                        {
                            name: 'Pythons'
                        },
                        {
                            name: 'Penguins'
                        }
                    ]
                },
                {
                    name: 'Dashboard'
                },
                {
                    name: 'Animals',
                    subItems: [
                        {
                            name: 'Puppies'
                        },
                        {
                            name: 'Parakeets'
                        },
                        {
                            name: 'Pythons'
                        },
                        {
                            name: 'Parrots'
                        },
                        {
                            name: 'Peacocks'
                        },
                        {
                            name: 'Penguins'
                        }
                    ]
                },
                {
                    name: 'Mordant'
                },
                {
                    name: 'All Pages'
                }
            ]

[alitaheri]

it cannot be a bug with react. and I can't reproduce this on my end. any chance you can write a small failing test so we can make it pass? I looked at the code, if there is anything wrong it's not related to this PR anyway as it,s not where the logic actually happens. So it's going to be hard to fix. A failing test will be the fastest way to squeeze out this bug. And make sure it will never happen again 👍

[Dreculah]

OK, thanks! I've asked one of my teammates to have a go at this.

[alitaheri]

That's great news. thank you and your teammate for looking into this 👍 👍

[Dreculah]

Thank you for material-ui!

[alitaheri]

You should thank callemall and @hai-cea, I'm just a simple collaborator 😅

But I'm really glad this project is helping your company 👍 👍

[cfkenandy]

Hi @alitaheri , I’m working with @Dreculah . I have created 2 failing test cases for the
nested menu items. You can find the project and the testcase at: nestedMenuItems.zip
After you npm install, you should be able to run the test using ’npm run test’ . ‘test/Main_test.jsx’ contains the test cases. Let me know if you need more information or any help with this.

[alitaheri]

@cfkenandy Wow that's awesome thank you 👍 any chance you can turn that zip into a PR so that others can check it out with ease? That would be awesome. 😍

[cfkenandy]

@alitaheri , You mean I should create a branch from https://github.com/callemall/material-ui/ and then create a PR?

[alitaheri]

@cfkenandy You should fork this repository, then create a branch on your fork, then update your branch with the failing tests and then submit a PR wanting to merge that branch from your fork onto the master branch of this repository.

[mbrookes]

@cfkenandy Here's one approach:

https://github.com/callemall/material-ui/blob/master/CONTRIBUTING.md#submitting-a-pull-request

[cfkenandy]

Thanks! Am on it.

[cfkenandy]

@alitaheri Hope I got this right. #3573

[alitaheri]

@cfkenandy Yes you did. thank you 👍 👍