feat(Menu): Make Submenu open state controlled #1125
Conversation
sophieH29
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1125 +/- ##
==========================================
- Coverage 82.38% 82.33% -0.05%
==========================================
Files 730 730
Lines 8685 8684 -1
Branches 1167 1232 +65
==========================================
- Hits 7155 7150 -5
- Misses 1515 1519 +4
Partials 15 15
Continue to review full report at Codecov.
|
…dust-ui/react into feat/submenu-autocontrolled
| */ | ||
| trySetState = (maybeState, state?) => { | ||
| trySetState = (state: Partial<S>, callback?: () => void) => { | ||
| const { autoControlledProps } = this.constructor as any | ||
| if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
This check should be removed, otherwise we will get incorrect warnings
| ...this.props, | ||
| menuOpen: newValue, | ||
| }) | ||
| } |
There was a problem hiding this comment.
I don't like this method, but I don't have a better option now...
| key: '3', | ||
| content: 'item3', | ||
| menu: [{ key: '1', content: 'item3.1' }, { key: '2', content: 'item3.2' }], | ||
| }, |
There was a problem hiding this comment.
I suggest to make it more compact:
menu: [
{ key: '1', content: 'item1' },
{ key: '2', content: 'item2' },
{ key: '3', content: 'item3' },
],
There was a problem hiding this comment.
As it will become more compact, do we need renderItems() at all?
| <span>{`Editorials menu item requested to change its submenu open state to "${ | ||
| this.state.editorialsMenuOpen | ||
| }".`}</span> | ||
| <Menu defaultActiveIndex={0} items={this.renderItems()} /> |
There was a problem hiding this comment.
This looks unreadable 🤔
What about something like this:
<Header as="h5" content="Current state:" />
<pre>{JSON.stringify(this.state, null, 2)}</pre>
<Divider />
<Menu defaultActiveIndex={0} items={this.renderItems()} />
| /> | ||
| } | ||
| /> | ||
| ) |
There was a problem hiding this comment.
Should we revert these changes?
There was a problem hiding this comment.
oops, yeah. Did some testing, great you noticed
| _.invoke(this.props, 'onActiveChanged', e, { ...this.props, active: true }) | ||
| e.stopPropagation() | ||
| e.preventDefault() | ||
| } | ||
| } | ||
|
|
||
| private trySetMenuOpen(newValue: boolean, eventArgs: any, onStateChanged?: any) { |
There was a problem hiding this comment.
| private trySetMenuOpen(newValue: boolean, eventArgs: any, onStateChanged?: any) { | |
| private trySetMenuOpen(e: React.SyntethicEvent, newValue: boolean, onStateChanged?: any) { |
nit: Can we place e param on the first place as in other methods?
There was a problem hiding this comment.
fyi, the order is the same as in Popup for trySetOpen method.
And the other thing, if you would want to change state without event, you would have to call
trySetMenuOpen(null, newValue)
instead of trySetMenuOpen(newValue). I would leave an order as it is as I think the newValue param has biggest priority than event object
| if (state) newState = { ...newState, ...state } | ||
|
|
||
| if (Object.keys(newState).length > 0) this.setState(newState) | ||
| if (Object.keys(newState).length > 0) this.setState(newState, callback) |
There was a problem hiding this comment.
Not sure, that we really need this condition, but let's leave it as is for now
This PR aims make submenu open state controlled, thus to bring the flexibility of using submenu and rewrite default behavior.