feat(Menu): Add `items` shorthand #506

layershifter · 2016-09-17T18:05:15Z

This PR:

Resolved old TODOs for Menu in docs;
Adds items shorthand for Menu:

const items [
  {name: 'Open'},
  {name: 'Save'},
]

<Menu items={items} onItemClick={handler} />

const items [
  {name: 'Open', onClick: onOpenClick},
  {active: true, name: 'Save', onClick: onSaveClick},
]

<Menu items={items} />

levithomason · 2016-09-18T18:37:53Z

Looks like tests crashed, Maximum call stack exceeded. I've retried tests without cache to see if it resolves the issue.

There are also a number of new lines added without coverage. I suspect codecov will fail unless tests are added for the red lines here:

FWIW, you can see coverage info right on the PR with the codecov browser extension (shown above). Also, you can open /coverage/lcov-report/index.html and drill down into specific coverage info.

jeffcarbs · 2016-09-19T01:53:00Z

src/collections/Menu/Menu.js

+  renderItems() {
+    const { items, onItemClick } = this.props
+
+    this.trySetState({ activeIndex: _.findIndex(items, ['active', false]) || -1 })


I don't think there should be a setState call inside a render.

Agree, moved to componentWillMount

jeffcarbs · 2016-09-19T01:57:59Z

src/collections/Menu/Menu.js

+      const active = this.state.activeIndex === index
+      const finalKey = childKey || [content, name].join('-')
+
+      const handleClick = (e, { itemName, itemIndex }) => {


Prop changes generally result in a component re-rendering. Since this function is being creating new for each item each time renderItems is called, these MenuItems will always be receiving new props on each render. This should probably use a similar pattern of having Menu define a handleItemClick callback like the Dropdown does.

levithomason · 2016-09-19T15:02:07Z

src/collections/Menu/Menu.js

+      const finalKey = childKey || [content, name].join('-')
+
+      const handleClick = (e, { itemName, itemIndex }) => {
+        const handler = onItemClick || onClick


If the user passes both a Menu onItemClick and an Item onClick, their onClick will never be called. Instead of choosing one or the other, I think we should call each separately:

if (onClick) onClick(e, { itemName, itemIndex }) if (onItemClick) onItemClick(e, { itemName, itemIndex })

I've also called them in order here, from the Item up to the Menu.

layershifter · 2016-09-19T15:14:01Z

src/collections/Menu/Menu.js

-    }
-
-    return <ElementType {...rest} className={classes}>{this.renderItems()}</ElementType>
+    return <ElementType {...rest} className={classes}>{children || this.renderItems()}</ElementType>


More simple

layershifter · 2016-09-19T15:16:27Z

src/collections/Menu/Menu.js

+
+      const handleClick = (e, data) => {
+        this.handleItemClick(e, data)
+        if (onClick) onClick(e, data)
      }


If the user passes both a Menu onItemClick and an Item onClick

Fixed there, but still

Since this function is being creating new for each item each time renderItems is called, these MenuItems will always be receiving new props on each render.

What I can do there?

Anything that doesn't recreate the handleClick function every loop would work. You could for instance pass the onClick as an extra parameter to the handleItemClick method:

this.handleItemClick(e, data, onClick)

Then, in handleItemClick if onClick exists, call it with the same event and data.

I'm moved condition to handleItemClick.

handleItemClick(e, { name, index }, onClick) { ... if (onClick) onClick(e, { name, index }) if (onItemClick) onItemClick(e, { name, index }) }

But problem will be still there, handleClick will be created on each iteration, or not?

const handleClick = (e, data) => this.handleItemClick(e, data, onClick) return <MenuItem onClick={handleClick} /> // return <MenuItem onClick={(e, data) => this.handleItemClick(e, data, onClick)} />

Sorry slow brain today, you are correct. It looks like if you are going to call two functions, then we'll always have to create the callback function in every loop. If we weren't calling both handleItemClick and onClick, then we could just pass handleItemClick.

As I write this, one other idea comes to mind. You could just pass onClick={this.handleItemClick} which will get called with the item name/index. Then, you could lookup the item in items and call its onClick with the same args if it exists:

handleItemClick = (e, { name, index }) => { const { items, onItemClick } = this.props if (_.get(items, `[${index}].onClick`)) items[index].onClick(e, { name, index }) // ... }

layershifter · 2016-09-19T15:18:17Z

There are also a number of new lines added without coverage.

I'll improve coverage and add docs soon.

codecov-io · 2016-09-19T16:46:51Z

Current coverage is 98.74% (diff: 100%)

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

@@             master       #506   diff @@
==========================================
  Files           106        106          
  Lines          1744       1753     +9   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1722       1731     +9   
  Misses           22         22          
  Partials          0          0

Powered by Codecov. Last update fa64096...186eada

…om/TechnologyAdvice/stardust into feat/menu-shorthand

layershifter · 2016-09-20T07:24:41Z

Updated to master, seems we ready to go.

levithomason · 2016-09-20T15:27:17Z

Reviewing, will merge if I can

levithomason · 2016-09-20T15:33:45Z

Just cleaned up one unused param, will merge when tests pass.

levithomason · 2016-09-20T15:44:53Z

Released in stardust@0.44.9, thanks much!

Alexander Fedyashov added 2 commits September 17, 2016 20:39

fix(Menu): Complete update TODOs

e3df244

feat(Menu): Add items shorthand

c97dec9

layershifter added the enhancement label Sep 17, 2016

jeffcarbs reviewed Sep 19, 2016

View reviewed changes

levithomason reviewed Sep 19, 2016

View reviewed changes

feat(Menu): Add items shorthand

17cd49c

layershifter commented Sep 19, 2016

View reviewed changes

feat(Menu): Add items shorthand

c77b459

layershifter added the ready for review label Sep 19, 2016

Alexander Fedyashov added 2 commits September 19, 2016 20:16

feat(Menu): Add items shorthand

8d04e33

Merge branches 'feat/menu-shorthand' and 'master' of https://github.c…

2a3283d

…om/TechnologyAdvice/stardust into feat/menu-shorthand

fix(Menu): remove unused param onClick

186eada

levithomason merged commit bce49c9 into master Sep 20, 2016

levithomason deleted the feat/menu-shorthand branch September 20, 2016 15:39

levithomason removed the ready for review label Sep 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Menu): Add `items` shorthand #506

feat(Menu): Add `items` shorthand #506

layershifter commented Sep 17, 2016 •

edited

Loading

levithomason commented Sep 18, 2016

jeffcarbs Sep 19, 2016

layershifter Sep 19, 2016

jeffcarbs Sep 19, 2016

levithomason Sep 19, 2016

layershifter Sep 19, 2016

layershifter Sep 19, 2016

levithomason Sep 19, 2016

layershifter Sep 19, 2016

levithomason Sep 19, 2016 •

edited

Loading

layershifter commented Sep 19, 2016

codecov-io commented Sep 19, 2016 •

edited

Loading

layershifter commented Sep 20, 2016

levithomason commented Sep 20, 2016

levithomason commented Sep 20, 2016

levithomason commented Sep 20, 2016

feat(Menu): Add items shorthand #506

feat(Menu): Add items shorthand #506

Conversation

layershifter commented Sep 17, 2016 • edited Loading

levithomason commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

layershifter commented Sep 19, 2016

codecov-io commented Sep 19, 2016 • edited Loading

Current coverage is 98.74% (diff: 100%)

layershifter commented Sep 20, 2016

levithomason commented Sep 20, 2016

levithomason commented Sep 20, 2016

levithomason commented Sep 20, 2016

feat(Menu): Add `items` shorthand #506

feat(Menu): Add `items` shorthand #506

layershifter commented Sep 17, 2016 •

edited

Loading

levithomason Sep 19, 2016 •

edited

Loading

codecov-io commented Sep 19, 2016 •

edited

Loading