Skip to content

Commit bda567f

Browse files
bpas247taion
bpas247
authored and
taion
committed
fix(AbstractNav): allow passed in refs to be properly forwarded (#4031)
* fix: first pathrough of migration to AbstractNav forwarding ref does not currently work, due to various bugs * Various fixes for context functionality for AbstractNav * Apply suggestions from code review Co-Authored-By: Jimmy Jia <[email protected]> * Migrate AbstractNav implementation to use hooks in replace of HOC This reduces the complexity of the AbstractNav implementation, especially in regards to how the contexts are integrated with it. * fix: Nav and TabContainer selectors not finding the right element This fixes the assertion issues caused by the selectors testing implementation details to find the DOM elements, rather than testing for something that is gauranteed to be in the implementation (E.g. the class `nav` on the Nav component, since it's part of the Bootstrap design spec) of the rendered DOM element. * fix: AbstractNav not properly merging both of its refs Currently, we use a ref to gain access to the underlying component to implement our own functionality. However, this causes an issue with not allowing the user to forward their own ref to gain access to the underlying component. using `useMergedRefs`, we are able to forward both refs to the underlying component. * use useMergedHooks from restart/hooks
1 parent 9993869 commit bda567f

File tree

5 files changed

+124
-147
lines changed

5 files changed

+124
-147
lines changed

‎package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@
122122
"@babel/runtime": "^7.4.2",
123123
"@react-bootstrap/react-popper": "1.2.1",
124124
"@restart/context": "^2.1.4",
125-
"@restart/hooks": "^0.3.0",
125+
"@restart/hooks": "^0.3.11",
126126
"classnames": "^2.2.6",
127127
"dom-helpers": "^3.4.0",
128128
"invariant": "^2.2.4",

‎src/AbstractNav.js

+116-131
Original file line numberDiff line numberDiff line change
@@ -1,154 +1,139 @@
1-
import React from 'react';
1+
import React, { useEffect, useState, useRef, useContext } from 'react';
22
import qsa from 'dom-helpers/query/querySelectorAll';
33
import PropTypes from 'prop-types';
4+
import useMergedRefs from '@restart/hooks/useMergedRefs';
45

5-
import mapContextToProps from '@restart/context/mapContextToProps';
66
import SelectableContext, { makeEventKey } from './SelectableContext';
77
import NavContext from './NavContext';
88
import TabContext from './TabContext';
99

1010
const noop = () => {};
1111

12-
class AbstractNav extends React.Component {
13-
static propTypes = {
14-
onSelect: PropTypes.func.isRequired,
15-
16-
as: PropTypes.elementType,
17-
18-
/** @private */
19-
onKeyDown: PropTypes.func,
20-
/** @private */
21-
parentOnSelect: PropTypes.func,
22-
/** @private */
23-
getControlledId: PropTypes.func,
24-
/** @private */
25-
getControllerId: PropTypes.func,
26-
/** @private */
27-
activeKey: PropTypes.any,
28-
};
29-
30-
state = {
31-
navContext: null,
32-
};
33-
34-
static getDerivedStateFromProps({
35-
activeKey,
36-
getControlledId,
37-
getControllerId,
38-
role,
39-
}) {
40-
return {
41-
navContext: {
42-
role, // used by NavLink to determine it's role
43-
activeKey: makeEventKey(activeKey),
44-
getControlledId: getControlledId || noop,
45-
getControllerId: getControllerId || noop,
46-
},
47-
};
48-
}
49-
50-
componentDidUpdate() {
51-
if (!this._needsRefocus || !this.listNode) return;
52-
53-
let activeChild = this.listNode.querySelector('[data-rb-event-key].active');
54-
if (activeChild) activeChild.focus();
55-
}
56-
57-
getNextActiveChild(offset) {
58-
if (!this.listNode) return null;
59-
60-
let items = qsa(this.listNode, '[data-rb-event-key]:not(.disabled)');
61-
let activeChild = this.listNode.querySelector('.active');
62-
63-
let index = items.indexOf(activeChild);
64-
if (index === -1) return null;
65-
66-
let nextIndex = index + offset;
67-
if (nextIndex >= items.length) nextIndex = 0;
68-
if (nextIndex < 0) nextIndex = items.length - 1;
69-
return items[nextIndex];
70-
}
71-
72-
handleSelect = (key, event) => {
73-
const { onSelect, parentOnSelect } = this.props;
74-
if (key == null) return;
75-
if (onSelect) onSelect(key, event);
76-
if (parentOnSelect) parentOnSelect(key, event);
77-
};
78-
79-
handleKeyDown = event => {
80-
const { onKeyDown } = this.props;
81-
if (onKeyDown) onKeyDown(event);
82-
83-
let nextActiveChild;
84-
switch (event.key) {
85-
case 'ArrowLeft':
86-
case 'ArrowUp':
87-
nextActiveChild = this.getNextActiveChild(-1);
88-
break;
89-
case 'ArrowRight':
90-
case 'ArrowDown':
91-
nextActiveChild = this.getNextActiveChild(1);
92-
break;
93-
default:
94-
return;
95-
}
96-
if (!nextActiveChild) return;
12+
const propTypes = {
13+
onSelect: PropTypes.func.isRequired,
14+
15+
as: PropTypes.elementType,
16+
17+
role: PropTypes.string,
9718

98-
event.preventDefault();
99-
this.handleSelect(nextActiveChild.dataset.rbEventKey, event);
100-
this._needsRefocus = true;
101-
};
19+
/** @private */
20+
onKeyDown: PropTypes.func,
21+
/** @private */
22+
parentOnSelect: PropTypes.func,
23+
/** @private */
24+
getControlledId: PropTypes.func,
25+
/** @private */
26+
getControllerId: PropTypes.func,
27+
/** @private */
28+
activeKey: PropTypes.any,
29+
};
10230

103-
attachRef = ref => {
104-
this.listNode = ref;
105-
};
31+
const defaultProps = {
32+
role: 'tablist',
33+
};
10634

107-
render() {
108-
const {
35+
const AbstractNav = React.forwardRef(
36+
(
37+
{
10938
// Need to define the default "as" during prop destructuring to be compatible with styled-components github.com/react-bootstrap/react-bootstrap/issues/3595
11039
as: Component = 'ul',
111-
onSelect: _,
112-
parentOnSelect: _0,
113-
getControlledId: _1,
114-
getControllerId: _2,
115-
activeKey: _3,
40+
onSelect,
41+
activeKey,
42+
role,
43+
onKeyDown,
11644
...props
117-
} = this.props;
118-
119-
if (props.role === 'tablist') {
120-
props.onKeyDown = this.handleKeyDown;
45+
},
46+
ref,
47+
) => {
48+
const parentOnSelect = useContext(SelectableContext);
49+
const tabContext = useContext(TabContext);
50+
51+
let getControlledId, getControllerId;
52+
53+
if (tabContext) {
54+
activeKey = tabContext.activeKey;
55+
getControlledId = tabContext.getControlledId;
56+
getControllerId = tabContext.getControllerId;
12157
}
12258

59+
const [needsRefocus, setRefocus] = useState(false);
60+
61+
const listNode = useRef(null);
62+
63+
const getNextActiveChild = offset => {
64+
if (!listNode.current) return null;
65+
66+
let items = qsa(listNode.current, '[data-rb-event-key]:not(.disabled)');
67+
let activeChild = listNode.current.querySelector('.active');
68+
69+
let index = items.indexOf(activeChild);
70+
if (index === -1) return null;
71+
72+
let nextIndex = index + offset;
73+
if (nextIndex >= items.length) nextIndex = 0;
74+
if (nextIndex < 0) nextIndex = items.length - 1;
75+
return items[nextIndex];
76+
};
77+
78+
const handleSelect = (key, event) => {
79+
if (key == null) return;
80+
if (onSelect) onSelect(key, event);
81+
if (parentOnSelect) parentOnSelect(key, event);
82+
};
83+
84+
const handleKeyDown = event => {
85+
if (onKeyDown) onKeyDown(event);
86+
87+
let nextActiveChild;
88+
switch (event.key) {
89+
case 'ArrowLeft':
90+
case 'ArrowUp':
91+
nextActiveChild = getNextActiveChild(-1);
92+
break;
93+
case 'ArrowRight':
94+
case 'ArrowDown':
95+
nextActiveChild = getNextActiveChild(1);
96+
break;
97+
default:
98+
return;
99+
}
100+
if (!nextActiveChild) return;
101+
102+
event.preventDefault();
103+
handleSelect(nextActiveChild.dataset.rbEventKey, event);
104+
setRefocus(true);
105+
};
106+
107+
useEffect(() => {
108+
if (listNode.current && needsRefocus) {
109+
let activeChild = listNode.current.querySelector(
110+
'[data-rb-event-key].active',
111+
);
112+
113+
if (activeChild) activeChild.focus();
114+
}
115+
}, [listNode, needsRefocus]);
116+
117+
const mergedRef = useMergedRefs(ref, listNode);
118+
123119
return (
124-
<SelectableContext.Provider value={this.handleSelect}>
125-
<NavContext.Provider value={this.state.navContext}>
126-
<Component
127-
{...props}
128-
onKeyDown={this.handleKeyDown}
129-
ref={this.attachRef}
130-
/>
120+
<SelectableContext.Provider value={handleSelect}>
121+
<NavContext.Provider
122+
value={{
123+
role, // used by NavLink to determine it's role
124+
activeKey: makeEventKey(activeKey),
125+
getControlledId: getControlledId || noop,
126+
getControllerId: getControllerId || noop,
127+
}}
128+
>
129+
<Component {...props} onKeyDown={handleKeyDown} ref={mergedRef} />
131130
</NavContext.Provider>
132131
</SelectableContext.Provider>
133132
);
134-
}
135-
}
136-
137-
export default mapContextToProps(
138-
[SelectableContext, TabContext],
139-
(parentOnSelect, tabContext, { role }) => {
140-
if (!tabContext) return { parentOnSelect };
141-
142-
const { activeKey, getControllerId, getControlledId } = tabContext;
143-
return {
144-
activeKey,
145-
parentOnSelect,
146-
role: role || 'tablist',
147-
// pass these two through to avoid having to listen to
148-
// both Tab and Nav contexts in NavLink
149-
getControllerId,
150-
getControlledId,
151-
};
152133
},
153-
AbstractNav,
154134
);
135+
136+
AbstractNav.propTypes = propTypes;
137+
AbstractNav.defaultProps = defaultProps;
138+
139+
export default AbstractNav;

‎test/NavSpec.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ describe('<Nav>', () => {
217217
</Nav>,
218218
);
219219

220-
wrapper.assertSingle('div[role="tablist"]');
220+
wrapper.assertSingle('.nav[role="tablist"]');
221221
wrapper.find('a[role="tab"]').length.should.equal(2);
222222
});
223223
});

‎test/TabContainerSpec.js

+2-10
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,7 @@ describe('<TabContainer>', () => {
8787
</TabContainer>,
8888
);
8989

90-
instance
91-
.find('Nav')
92-
.getDOMNode()
93-
.getAttribute('role')
94-
.should.equal('tablist');
90+
instance.assertSingle('.nav[role="tablist"]');
9591

9692
instance
9793
.find('NavLink a')
@@ -116,11 +112,7 @@ describe('<TabContainer>', () => {
116112
</TabContainer>,
117113
);
118114

119-
instance
120-
.find('Nav')
121-
.getDOMNode()
122-
.getAttribute('role')
123-
.should.equal('navigation');
115+
instance.assertSingle('.nav[role="navigation"]');
124116

125117
// make sure its not passed to the Nav.Link
126118
expect(

‎yarn.lock

+4-4
Original file line numberDiff line numberDiff line change
@@ -913,10 +913,10 @@
913913
resolved "https://registry.yarnpkg.com/@restart/context/-/context-2.1.4.tgz#a99d87c299a34c28bd85bb489cb07bfd23149c02"
914914
integrity sha512-INJYZQJP7g+IoDUh/475NlGiTeMfwTXUEr3tmRneckHIxNolGOW9CTq83S8cxq0CgJwwcMzMJFchxvlwe7Rk8Q==
915915

916-
"@restart/hooks@^0.3.0":
917-
version "0.3.9"
918-
resolved "https://registry.yarnpkg.com/@restart/hooks/-/hooks-0.3.9.tgz#b2849bee3faec1d3fc531fdf301f4b27649baa8e"
919-
integrity sha512-6gL84qcdZHUN0V5czyMXzAbcBBpHm8H5Gwj7eqnVi6p3JzGwJ5m2at19dKE9Gv3SsU3Hv9SPDX+6zQSExCjjkQ==
916+
"@restart/hooks@^0.3.11":
917+
version "0.3.11"
918+
resolved "https://registry.yarnpkg.com/@restart/hooks/-/hooks-0.3.11.tgz#79ef8b872710838fac8c540f499915ed97935a7a"
919+
integrity sha512-EcKbmMEMJBGNXl2u5WLpwmQnH47+USJW4wIQlwkStmsdt/igq+EUJX7tp0fha0GGIliJoxg69FSe06i5/B6HpA==
920920

921921
"@samverschueren/stream-to-observable@^0.3.0":
922922
version "0.3.0"

0 commit comments

Comments
 (0)