Skip to content

Commit 1d20de0

Browse files
canovacanova
authored
Change the selectedItem state to keep the index instead of the React element (Merge PR #25)
2 parents 99a9f77 + 216ff2c commit 1d20de0

File tree

2 files changed

+194
-29
lines changed

2 files changed

+194
-29
lines changed

‎src/AbstractMenu.js‎

Lines changed: 65 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -131,50 +131,86 @@ export default class AbstractMenu extends Component {
131131
return i === currentIndex ? null : i;
132132
}
133133

134-
const currentIndex = children.indexOf(selectedItem);
134+
const currentIndex = selectedItem ? selectedItem.index : -1;
135135
const nextEnabledChildIndex = findNextEnabledChildIndex(currentIndex);
136136

137137
if (nextEnabledChildIndex !== null) {
138138
this.setState({
139-
selectedItem: children[nextEnabledChildIndex],
139+
selectedItem: {
140+
index: nextEnabledChildIndex,
141+
// We need to know the type of the selected item, so we can
142+
// check it during render and tryToOpenSubMenu.
143+
type: children[nextEnabledChildIndex].type
144+
},
140145
forceSubMenuOpen: false
141146
});
142147
}
143148
};
144149

145-
onChildMouseMove = (child) => {
146-
if (this.state.selectedItem !== child) {
147-
this.setState({ selectedItem: child, forceSubMenuOpen: false });
150+
onChildMouseMove = (child, itemIndex) => {
151+
if (this.state.selectedItem === null || this.state.selectedItem.index !== itemIndex) {
152+
this.setState({
153+
selectedItem: {
154+
index: itemIndex,
155+
type: child.type
156+
},
157+
forceSubMenuOpen: false
158+
});
148159
}
149160
};
150161

151162
onChildMouseLeave = () => {
152163
this.setState({ selectedItem: null, forceSubMenuOpen: false });
153164
};
154165

155-
renderChildren = children => React.Children.map(children, (child) => {
156-
const props = {};
157-
if (!React.isValidElement(child)) return child;
158-
if ([MenuItem, this.getSubMenuType()].indexOf(child.type) < 0) {
159-
// Maybe the MenuItem or SubMenu is capsuled in a wrapper div or something else
160-
props.children = this.renderChildren(child.props.children);
161-
return React.cloneElement(child, props);
162-
}
163-
props.onMouseLeave = this.onChildMouseLeave.bind(this);
164-
if (child.type === this.getSubMenuType()) {
165-
// special props for SubMenu only
166-
props.forceOpen = this.state.forceSubMenuOpen && (this.state.selectedItem === child);
167-
props.forceClose = this.handleForceClose;
168-
props.parentKeyNavigationHandler = this.handleKeyNavigation;
169-
}
170-
if (!child.props.divider && this.state.selectedItem === child) {
171-
// special props for selected item only
172-
props.selected = true;
173-
props.ref = (ref) => { this.seletedItemRef = ref; };
166+
/**
167+
* Render all the children.
168+
* It has a `childIndexRef` parameter to be able to construct the child
169+
* indexes properly. A reference was needed for this function because this
170+
* is a recursive function that could mutate the index and pass it back to
171+
* the caller. That parameter should always be undefined while calling from
172+
* outside.
173+
* TODO: Rewrite this function in a way that we don't need this reference.
174+
*/
175+
renderChildren = (children, childIndexRef = { value: -1 }) =>
176+
React.Children.map(children, (child) => {
177+
let currentChildIndexRef = childIndexRef;
178+
const props = {};
179+
if (!React.isValidElement(child)) return child;
180+
181+
if ([MenuItem, this.getSubMenuType()].indexOf(child.type) < 0) {
182+
// Maybe the MenuItem or SubMenu is capsuled in a wrapper div or something else
183+
props.children = this.renderChildren(child.props.children, currentChildIndexRef);
184+
return React.cloneElement(child, props);
185+
}
186+
187+
// At this point we know that this is a menu item and we are going to
188+
// render it. We need to increment the child index and assign it as
189+
// the item index.
190+
let itemIndex = null;
191+
if (!child.props.divider) {
192+
// A MenuItem can be a divider. Do not increment the value if it's.
193+
itemIndex = ++currentChildIndexRef.value;
194+
}
195+
196+
props.onMouseLeave = this.onChildMouseLeave.bind(this);
197+
if (child.type === this.getSubMenuType()) {
198+
// special props for SubMenu only
199+
props.forceOpen = this.state.forceSubMenuOpen &&
200+
(this.state.selectedItem && this.state.selectedItem.index === itemIndex);
201+
props.forceClose = this.handleForceClose;
202+
props.parentKeyNavigationHandler = this.handleKeyNavigation;
203+
}
204+
if (!child.props.divider &&
205+
(this.state.selectedItem && this.state.selectedItem.index === itemIndex)) {
206+
// special props for selected item only
207+
props.selected = true;
208+
props.ref = (ref) => { this.seletedItemRef = ref; };
209+
return React.cloneElement(child, props);
210+
}
211+
212+
// onMouseMove is only needed for non selected items
213+
props.onMouseMove = () => this.onChildMouseMove(child, itemIndex);
174214
return React.cloneElement(child, props);
175-
}
176-
// onMouseMove is only needed for non selected items
177-
props.onMouseMove = () => this.onChildMouseMove(child);
178-
return React.cloneElement(child, props);
179-
});
215+
});
180216
}

‎tests/ContextMenu.test.js‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import React from 'react';
22
import { mount } from 'enzyme';
33

44
import ContextMenu from '../src/ContextMenu';
5+
import MenuItem from '../src/MenuItem';
56
import { showMenu, hideMenu } from '../src/actions';
67

78
describe('ContextMenu tests', () => {
@@ -194,4 +195,132 @@ describe('ContextMenu tests', () => {
194195
expect(onMouseLeave).toHaveBeenCalled();
195196
component.unmount();
196197
});
198+
199+
test('should select the proper menu items with down arrow', () => {
200+
const data = { position: { x: 50, y: 50 }, id: 'CORRECT_ID' };
201+
const onHide = jest.fn();
202+
const component = mount(
203+
<ContextMenu id={data.id} onHide={onHide}>
204+
<MenuItem onClick={jest.fn()}>Item 1</MenuItem>
205+
<MenuItem onClick={jest.fn()}>Item 2</MenuItem>
206+
</ContextMenu>
207+
);
208+
const downArrow = new window.KeyboardEvent('keydown', { keyCode: 40 });
209+
210+
showMenu(data);
211+
// Check that it's visible and there is no selected item at first.
212+
expect(component.state()).toEqual(
213+
Object.assign(
214+
{ isVisible: true, forceSubMenuOpen: false, selectedItem: null },
215+
data.position
216+
)
217+
);
218+
219+
// Select the first item with down arrow.
220+
document.dispatchEvent(downArrow);
221+
// Index 0 with MenuItem type should be selected.
222+
expect(component.state().selectedItem).toEqual({
223+
index: 0,
224+
type: MenuItem
225+
});
226+
227+
// Select the second item with down arrow.
228+
document.dispatchEvent(downArrow);
229+
// Index 1 with MenuItem type should be selected.
230+
expect(component.state().selectedItem).toEqual({
231+
index: 1,
232+
type: MenuItem
233+
});
234+
235+
// Select the next item. But since this was the last item, it should loop
236+
// back to the first again.
237+
document.dispatchEvent(downArrow);
238+
// Index 0 with MenuItem type should be selected.
239+
expect(component.state().selectedItem).toEqual({
240+
index: 0,
241+
type: MenuItem
242+
});
243+
244+
component.unmount();
245+
});
246+
247+
test('should select the proper menu items with up arrow', () => {
248+
const data = { position: { x: 50, y: 50 }, id: 'CORRECT_ID' };
249+
const onHide = jest.fn();
250+
const component = mount(
251+
<ContextMenu id={data.id} onHide={onHide}>
252+
<MenuItem onClick={jest.fn()}>Item 1</MenuItem>
253+
<MenuItem onClick={jest.fn()}>Item 2</MenuItem>
254+
</ContextMenu>
255+
);
256+
const upArrow = new window.KeyboardEvent('keydown', { keyCode: 38 });
257+
258+
showMenu(data);
259+
// Check that it's visible and there is no selected item at first.
260+
expect(component.state()).toEqual(
261+
Object.assign(
262+
{ isVisible: true, forceSubMenuOpen: false, selectedItem: null },
263+
data.position
264+
)
265+
);
266+
267+
// Select the previous item. But since there was nothing selected, it
268+
// should loop back down to the last item.
269+
document.dispatchEvent(upArrow);
270+
// Index 1 with MenuItem type should be selected.
271+
expect(component.state().selectedItem).toEqual({
272+
index: 1,
273+
type: MenuItem
274+
});
275+
276+
// Select the first item with up arrow.
277+
document.dispatchEvent(upArrow);
278+
// Index 0 with MenuItem type should be selected.
279+
expect(component.state().selectedItem).toEqual({
280+
index: 0,
281+
type: MenuItem
282+
});
283+
284+
component.unmount();
285+
});
286+
287+
test('should preserve the selected item after an enter', () => {
288+
const data = { position: { x: 50, y: 50 }, id: 'CORRECT_ID' };
289+
const onHide = jest.fn();
290+
const component = mount(
291+
<ContextMenu id={data.id} onHide={onHide}>
292+
<MenuItem onClick={jest.fn()} preventClose>Item 1</MenuItem>
293+
<MenuItem divider />
294+
<MenuItem onClick={jest.fn()} preventClose>Item 2</MenuItem>
295+
</ContextMenu>
296+
);
297+
const upArrow = new window.KeyboardEvent('keydown', { keyCode: 38 });
298+
const enter = new window.KeyboardEvent('keydown', { keyCode: 13 });
299+
300+
showMenu(data);
301+
// Check that it's visible and there is no selected item at first.
302+
expect(component.state()).toEqual(
303+
Object.assign(
304+
{ isVisible: true, forceSubMenuOpen: false, selectedItem: null },
305+
data.position
306+
)
307+
);
308+
309+
// Select the second item up arrow.
310+
document.dispatchEvent(upArrow);
311+
expect(component.state().selectedItem).toEqual({
312+
index: 1,
313+
type: MenuItem
314+
});
315+
316+
// Press enter to select it.
317+
document.dispatchEvent(enter);
318+
// The selected item should be preserved and not reset.
319+
expect(component.state().selectedItem).toEqual({
320+
index: 1,
321+
type: MenuItem
322+
});
323+
324+
component.unmount();
325+
});
197326
});

0 commit comments

Comments
 (0)