feat(Menu): Update to v1 API #382

layershifter · 2016-08-08T19:34:34Z

WIP

Update Menu to v1 API

levithomason · 2016-08-11T07:24:30Z

Linking #142 for fixing in this PR as well.

levithomason · 2016-08-13T09:03:03Z

Linking previous update PR #319.

codecov-io · 2016-08-14T08:50:12Z

Current coverage is 98.61% (diff: 100%)

Merging #382 into master will increase coverage by <.01%

@@             master       #382   diff @@
==========================================
  Files            99        101     +2   
  Lines          1438       1444     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1418       1424     +6   
  Misses           20         20          
  Partials          0          0

Powered by Codecov. Last update 8eee04c...f376f44

layershifter · 2016-08-17T18:28:03Z

src/collections/Menu/Menu.js


      if (isItem) {
        const onClick = (e) => {
-          this.handleItemClick(e, i)
+          if (isLink) this.handleItemClick(e, i)
          if (child.props.onClick) child.props.onClick(e, i)
        }


I'll take this part from Accordion component.

Can you review this fragment? It looks not good to me.

I don't think we should add the click handler if the user is using subcomponents. We did this initially elsewhere, but there is just no reliable way to determine which children we should add it to.

React.Children can only loop through the top level children. If the user uses their own menu item component, then we have no reliable type or displayName to identify which components are menu items. They may also use a higher order component or decorator on the MenuItem, in which case the menu item is no longer a top level child but a nested child wrapped in the decorator.

Because of this, I think the only reliable way to pass props to children is when using the <Menu items={} /> prop. In this case, we know we can add props to every object in the array. It is the best way I know of to manage the parent-child coupling.

More here: https://facebook.github.io/react/docs/context.html#parent-child-coupling

EDIT

If the user uses subcomponents, they will just have to wire their own click handlers. The same is true for our <Dropdown options={} /> vs <Dropdown><Dropdown.Item /></Dropdown> markup.

Take a look on basic example of Menu. After click on item it switches active class to this item. I've implemented it like it is in Accordion, but as you say this solution has problem.

I'm correctly understand that you suggest to use Content? But, as you answered earlier:

Context is an advanced and experimental feature. The API is likely to change in future releases.

I'm proposing two separate usage patterns.

items={} prop

My suggestion is that this usage will automatically manage the active item state:

<Menu items={[{}, {}, ...]} />

With this usage we can add the onClick handlers our selves and manage the active prop on the MenuItems internally. We can do this easily since we must map over all the items to create the MenuItems.

Subcomponents

This usage would not self manage the active item. The user needs to implement their own onClick handlers and set the active prop:

class MenuExample extends Component { handleItemClick = (name) => this.setState({ activeItem: name }) render() { const { activeItem } = this.state return ( <Menu> <Menu.Item active={activeItem === 'home'} onClick={this.handleItemClick.bind('home')} /> <Menu.Item active={activeItem === 'search'} onClick={this.handleItemClick.bind('search')} /> <Menu.Item active={activeItem === 'settings'} onClick={this.handleItemClick.bind('settings')} /> </Menu> ) } }

These ideas can be improved on, but I think we should only auto manage the active state if you use the items prop.

It's reasonable, but it produce warning of ESLint 😞

True, our eslint settings don't like bind in an event handler. This can be solved easily like so:

onClick={() => this.handleItemClick('home')}

It seems that ESLint is more intelligent that you think 😺

layershifter · 2016-08-17T18:34:38Z

offtop: How I can clean up these commits from PR?

levithomason · 2016-08-17T19:17:53Z

Looks like a rebase was done from an out of date master. Update your local master, and the rebase from there. On the fix/menu branch I'd do:

$ git fetch origin master
$ git rebase origin/master

EDIT
If you're remote name is not origin, then use your remote name for the main Stardust repo.

layershifter · 2016-08-17T19:27:44Z

Seems, I'll left it for you on merge 😰

levithomason · 2016-08-17T19:35:07Z

no problem, one moment while i merge and force push...

levithomason · 2016-08-17T19:55:42Z

Give it a look and see if it is correct now. I have a copy of the original branch just in case I hosed this one.

layershifter · 2016-08-17T20:06:54Z

Looks okay, thanks 👍 Waiting for your reply there.

layershifter · 2016-08-21T10:56:13Z

Menu widely uses Popup in docs. Are there any chance for its implementaion in near future?

layershifter · 2016-08-21T11:20:08Z

Fixed examples using <iframe>, how I can correctly deal with this?

For finishing Content and States part of docs I need #414.

levithomason · 2016-08-21T23:02:26Z

I paused the Form work to finish the augmentation. I hope to ship that today. I can do the popup next, or if you'd like to get a start on it we can tackle that one together.

levithomason · 2016-08-24T06:24:52Z

I've started the Tab component, #430. It is made of a Menu and Segments. I think if MenuItem's accept a name prop, it could help the Tab component. It would also help our onClick binding as we can callback with the Menu.Item name:

export default class Menus extends Component {
  handleItemClick = (e, name) => this.setState({ activeItem: name })

  render() {
    const { activeItem } = this.state

    return (
      <Menu>
        <Menu.Item
          name='browse'
          active={activeItem === 'browse'}
          onClick={this.handleItemClick}
        >
          Browse
        </Menu.Item>

        <Menu.Item
          name='submit'
          active={activeItem === 'submit'}
          onClick={this.handleItemClick}
        >
          Submit
        </Menu.Item>

        <Menu.Menu position='right'>
          <Menu.Item
            name='signup'
            active={activeItem === 'signup'}
            onClick={this.handleItemClick}
          >
            Sign Up
          </Menu.Item>

          <Menu.Item
            name='help'
            active={activeItem === 'help'}
            onClick={this.handleItemClick}
          >
            Help
          </Menu.Item>
        </Menu.Menu>
      </Menu>
    )
  }
}

It can be used as shorthand props to generate the menu item text. Same example as above but with shorthand MenuItem child text:

export default class Menus extends Component {
  handleItemClick = (e, name) => this.setState({ activeItem: name })

  render() {
    const { activeItem } = this.state

    return (
      <Menu>
        <Menu.Item
          name='browse'
          active={activeItem === 'browse'}
          onClick={this.handleItemClick}
        />
        <Menu.Item
          name='submit'
          active={activeItem === 'submit'}
          onClick={this.handleItemClick}
        />
        <Menu.Menu position='right'>
          <Menu.Item
            name='signup'
            active={activeItem === 'signup'}
            onClick={this.handleItemClick}
          />
          <Menu.Item
            name='help'
            active={activeItem === 'help'}
            onClick={this.handleItemClick}
          /></Menu.Menu>
      </Menu>
    )
  }
}

Lastly, it works well with a shorthand items array where the objects are spread on each item. It could also be used to propagate the onClick to the Menu when using an array of items.

This would be an AutoControlledComponent now. The active item can be self managed. Optionally, a user could override the active item with an activeItemName prop:

const items = [
  { name: 'browse' },
  { name: 'submit' },
  { name: 'signup' },
  { name: 'help' },
]

export default class Menus extends Component {
  handleClick = (e, name) => {
    // do something with the clicked item
  }

  render() {
    return (
      <Menu onClick={this.handleClick} items={items} />
    )
  }
}

layershifter · 2016-08-24T11:01:03Z

Looks pretty, take it work.

layershifter · 2016-08-24T16:35:45Z

Added. I'm having problems with docs build on Windows, on master branch, too. And idea?

levithomason · 2016-08-24T16:58:54Z

Gah, yep! There are hardcoded /s in the paths. Windows uses \s :/ I don't have a quick solve for this off the top of my head.

The docgenInfo.json is generated in node during the build. The ComponentDoc components then import and parse the json in browser. So, the browser code needs to know to use / instead of \.

One idea might be setting a global platform dependent __PATH_SEP__ during the build.

// config.js
compiler_globals: {
  __PATH_SEP__: JSON.stringify(path.sep),
}

https://nodejs.org/api/path.html#path_path_sep

Then we could use __PATH_SEP__ in the doc build when building paths. If you like, please open another issue and we'll work on this there.

levithomason · 2016-08-24T17:35:51Z

Merged #435, give that a shot!

levithomason · 2016-08-28T14:58:42Z

In addition to calling back with the menu item name onClick, let's also call back with the item index:

handleClick(e, { name, index}) {
  //... 
} 

<Menu.Item onClick={this.handleClick} />

This will help other components like Tab and Accordion use the Menu to control their activeIndex.

layershifter · 2016-08-28T15:23:42Z

How I can get index when user passes children directly?

<Menu>
  <Menu.Item>A</Menu.Item>
  <Menu.Item>B</Menu.Item>
</Menu>

levithomason · 2016-09-02T00:29:53Z

I've updated this to the latest master. I also fixed the existing tests and added missing sub component tests. It should be all set for continuing with the docs.

Since I squashed and force pushed to clean it up a bit, you'll probably want to blow away your local branch in favor of this one.

I did not make any major updates to the functionality. So, the Menu itself is still in progress.

layershifter · 2016-09-02T06:14:17Z

Thanks much, I'll think I'll finish docs on this weekend.

layershifter · 2016-09-03T16:48:49Z

docs/app/Examples/collections/Menu/Variations/ColoredInvertedMenus.js

+  return <div>{menus}</div>
+}
+
+export default ColoredInvertedMenus


@levithomason what you think about this?

The loop is perfectly acceptable, but let's not show use of the private _meta prop. I don't want users knowing/using that. I think putting it in the docs will encourage use of it. Instead, let's just list the colors in an array explicitly.

layershifter · 2016-09-04T14:51:10Z

Docs

Unbelievable, but it is finished ✌️ We need review for them.

Tests

Tests will be finished tomorrow. And final review will there soon 👍

Shorthands

Discuss?

Menu.Menu

I think, we don't need provide items shorthand there because it's sub menu componen while shorthands cover only simplest cases.

Tests

There are here 🚢

Shorthands

I'll implement and add docs with tests soon 😪

levithomason · 2016-09-06T18:55:26Z

Will review this today, thanks for the massive amount of work here!

levithomason · 2016-09-07T00:45:28Z

Heads up, doing a final review and some minor tweaks. Hope to merge this today.

levithomason · 2016-09-07T02:32:53Z

Latest updates:

Start Case Name prop

Updated <Menu.Item name='fooBar' /> to Start Case the name (i.e. "Foo Bar") when used as content. Added a test for this and updated the examples as well.

Dropdown Item Updated

Completed TODO for Dropdown header usage. Also, reversed the augmentation from <Menu.Item as={Dropdown} /> to <Dropdown as={Menu.Item} /> as it seemed the Dropdown sub components made more sense this way. Finally, used icon prop shorthand for Dropdown.Items.

Colored Menus

Mapped over an array of colors to make examples a little more concise. More importantly, to not expose the _meta prop to users in the docs. I am sure it will lead to wider spread use of the _meta prop which is private.

Misc

I made some trivial updates to the docs in terms of verbiage and/or organization.

levithomason · 2016-09-07T02:36:21Z

🎉 This is great, thanks a million. We can do shorthand separately since this PR was so massive.

levithomason · 2016-09-07T02:39:26Z

Released in [email protected]

levithomason mentioned this pull request Aug 8, 2016

Update legacy component APIs to the v1 API #269

Closed

14 tasks

layershifter reviewed Aug 17, 2016
View reviewed changes

layershifter mentioned this pull request Aug 17, 2016

RFC: Consider component augmentation #403

Closed

levithomason force-pushed the fix/menu branch from f39a746 to f745b50 Compare August 17, 2016 19:54

levithomason changed the title ~~Update Menu to v1 API~~ Menu: Update to v1 API Aug 18, 2016

levithomason mentioned this pull request Aug 18, 2016

feat(List): update to v1 API #311

Closed

layershifter force-pushed the fix/menu branch from 377465d to d2e0ab4 Compare August 21, 2016 10:01

levithomason added the enhancement label Aug 22, 2016

layershifter mentioned this pull request Aug 24, 2016

Docs: Generation issue of docgenInfo.json on Windows #434

Closed

levithomason force-pushed the fix/menu branch from 764d371 to b5e96d3 Compare September 2, 2016 00:28

layershifter reviewed Sep 3, 2016
View reviewed changes

layershifter mentioned this pull request Sep 6, 2016

feat(Dropdown): Add Header sub-component #471

Merged

layershifter and others added 9 commits September 6, 2016 17:06

fix(Menu) Initial update

257e41e

fix(Menu) Update utils usage

3874d1a

fix(Menu) More docs

e7a966a

fix(Menu): rebase to master

75aaf99

test(Menu): add, update, and fix tests

7cbacc1

docs(Menu): More docs and props

b42a37e

docs(Menu): More docs and props

0693d73

feat(Menu): Update tests

2945313

fix(MenuItem): add dangling comma

b8d0d14

levithomason force-pushed the fix/menu branch from 37f6873 to b8d0d14 Compare September 7, 2016 00:06

feat(Menu): Start Case name prop as text

f376f44

levithomason merged commit 378afd6 into master Sep 7, 2016

levithomason deleted the fix/menu branch September 7, 2016 02:35

layershifter mentioned this pull request Mar 1, 2019

fix(test): remove hardcoded magic numbers microsoft/fluent-ui-react#997

Merged

feat(Menu): Update to v1 API #382

feat(Menu): Update to v1 API #382

Conversation

layershifter commented Aug 8, 2016

WIP

levithomason commented Aug 11, 2016

levithomason commented Aug 13, 2016

codecov-io commented Aug 14, 2016 • edited Loading

Current coverage is 98.61% (diff: 100%)

layershifter Aug 17, 2016

Choose a reason for hiding this comment

levithomason Aug 17, 2016 • edited Loading

Choose a reason for hiding this comment

layershifter Aug 17, 2016

Choose a reason for hiding this comment

levithomason Aug 17, 2016 • edited Loading

Choose a reason for hiding this comment

items={} prop

Subcomponents

layershifter Aug 18, 2016

Choose a reason for hiding this comment

levithomason Aug 18, 2016

Choose a reason for hiding this comment

layershifter Aug 21, 2016

Choose a reason for hiding this comment

layershifter commented Aug 17, 2016

levithomason commented Aug 17, 2016 • edited Loading

layershifter commented Aug 17, 2016 • edited Loading

levithomason commented Aug 17, 2016

levithomason commented Aug 17, 2016

layershifter commented Aug 17, 2016

layershifter commented Aug 21, 2016 • edited by levithomason Loading

layershifter commented Aug 21, 2016

levithomason commented Aug 21, 2016

levithomason commented Aug 24, 2016 • edited Loading

layershifter commented Aug 24, 2016

layershifter commented Aug 24, 2016

levithomason commented Aug 24, 2016 • edited Loading

levithomason commented Aug 24, 2016

levithomason commented Aug 28, 2016

layershifter commented Aug 28, 2016

levithomason commented Sep 2, 2016 • edited Loading

layershifter commented Sep 2, 2016

layershifter Sep 3, 2016

Choose a reason for hiding this comment

levithomason Sep 5, 2016 • edited Loading

Choose a reason for hiding this comment

layershifter commented Sep 4, 2016

Docs

Tests

Shorthands

Menu.Menu

Menu

levithomason commented Sep 5, 2016

layershifter commented Sep 6, 2016

Tests

Shorthands

levithomason commented Sep 6, 2016

levithomason commented Sep 7, 2016

levithomason commented Sep 7, 2016

Start Case Name prop

Dropdown Item Updated

Colored Menus

Misc

levithomason commented Sep 7, 2016

levithomason commented Sep 7, 2016

codecov-io commented Aug 14, 2016 •

edited

Loading

levithomason Aug 17, 2016 •

edited

Loading

levithomason Aug 17, 2016 •

edited

Loading

levithomason commented Aug 17, 2016 •

edited

Loading

layershifter commented Aug 17, 2016 •

edited

Loading

layershifter commented Aug 21, 2016 •

edited by levithomason

Loading

levithomason commented Aug 24, 2016 •

edited

Loading

levithomason commented Aug 24, 2016 •

edited

Loading

levithomason commented Sep 2, 2016 •

edited

Loading

levithomason Sep 5, 2016 •

edited

Loading