Commit bda567fb authored by Brady Pascoe's avatar Brady Pascoe Committed by Jimmy Jia
Browse files

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: default avatarJimmy Jia <tesrin@gmail.com>

* 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
parent 99938694
import React from 'react';
import React, { useEffect, useState, useRef, useContext } from 'react';
import qsa from 'dom-helpers/query/querySelectorAll';
import PropTypes from 'prop-types';
import useMergedRefs from '@restart/hooks/useMergedRefs';
import mapContextToProps from '@restart/context/mapContextToProps';
import SelectableContext, { makeEventKey } from './SelectableContext';
import NavContext from './NavContext';
import TabContext from './TabContext';
const noop = () => {};
class AbstractNav extends React.Component {
static propTypes = {
onSelect: PropTypes.func.isRequired,
as: PropTypes.elementType,
/** @private */
onKeyDown: PropTypes.func,
/** @private */
parentOnSelect: PropTypes.func,
/** @private */
getControlledId: PropTypes.func,
/** @private */
getControllerId: PropTypes.func,
/** @private */
activeKey: PropTypes.any,
};
state = {
navContext: null,
};
static getDerivedStateFromProps({
activeKey,
getControlledId,
getControllerId,
role,
}) {
return {
navContext: {
role, // used by NavLink to determine it's role
activeKey: makeEventKey(activeKey),
getControlledId: getControlledId || noop,
getControllerId: getControllerId || noop,
},
};
}
componentDidUpdate() {
if (!this._needsRefocus || !this.listNode) return;
let activeChild = this.listNode.querySelector('[data-rb-event-key].active');
if (activeChild) activeChild.focus();
}
getNextActiveChild(offset) {
if (!this.listNode) return null;
let items = qsa(this.listNode, '[data-rb-event-key]:not(.disabled)');
let activeChild = this.listNode.querySelector('.active');
let index = items.indexOf(activeChild);
if (index === -1) return null;
let nextIndex = index + offset;
if (nextIndex >= items.length) nextIndex = 0;
if (nextIndex < 0) nextIndex = items.length - 1;
return items[nextIndex];
}
handleSelect = (key, event) => {
const { onSelect, parentOnSelect } = this.props;
if (key == null) return;
if (onSelect) onSelect(key, event);
if (parentOnSelect) parentOnSelect(key, event);
};
handleKeyDown = event => {
const { onKeyDown } = this.props;
if (onKeyDown) onKeyDown(event);
let nextActiveChild;
switch (event.key) {
case 'ArrowLeft':
case 'ArrowUp':
nextActiveChild = this.getNextActiveChild(-1);
break;
case 'ArrowRight':
case 'ArrowDown':
nextActiveChild = this.getNextActiveChild(1);
break;
default:
return;
}
if (!nextActiveChild) return;
const propTypes = {
onSelect: PropTypes.func.isRequired,
as: PropTypes.elementType,
role: PropTypes.string,
event.preventDefault();
this.handleSelect(nextActiveChild.dataset.rbEventKey, event);
this._needsRefocus = true;
};
/** @private */
onKeyDown: PropTypes.func,
/** @private */
parentOnSelect: PropTypes.func,
/** @private */
getControlledId: PropTypes.func,
/** @private */
getControllerId: PropTypes.func,
/** @private */
activeKey: PropTypes.any,
};
attachRef = ref => {
this.listNode = ref;
};
const defaultProps = {
role: 'tablist',
};
render() {
const {
const AbstractNav = React.forwardRef(
(
{
// Need to define the default "as" during prop destructuring to be compatible with styled-components github.com/react-bootstrap/react-bootstrap/issues/3595
as: Component = 'ul',
onSelect: _,
parentOnSelect: _0,
getControlledId: _1,
getControllerId: _2,
activeKey: _3,
onSelect,
activeKey,
role,
onKeyDown,
...props
} = this.props;
if (props.role === 'tablist') {
props.onKeyDown = this.handleKeyDown;
},
ref,
) => {
const parentOnSelect = useContext(SelectableContext);
const tabContext = useContext(TabContext);
let getControlledId, getControllerId;
if (tabContext) {
activeKey = tabContext.activeKey;
getControlledId = tabContext.getControlledId;
getControllerId = tabContext.getControllerId;
}
const [needsRefocus, setRefocus] = useState(false);
const listNode = useRef(null);
const getNextActiveChild = offset => {
if (!listNode.current) return null;
let items = qsa(listNode.current, '[data-rb-event-key]:not(.disabled)');
let activeChild = listNode.current.querySelector('.active');
let index = items.indexOf(activeChild);
if (index === -1) return null;
let nextIndex = index + offset;
if (nextIndex >= items.length) nextIndex = 0;
if (nextIndex < 0) nextIndex = items.length - 1;
return items[nextIndex];
};
const handleSelect = (key, event) => {
if (key == null) return;
if (onSelect) onSelect(key, event);
if (parentOnSelect) parentOnSelect(key, event);
};
const handleKeyDown = event => {
if (onKeyDown) onKeyDown(event);
let nextActiveChild;
switch (event.key) {
case 'ArrowLeft':
case 'ArrowUp':
nextActiveChild = getNextActiveChild(-1);
break;
case 'ArrowRight':
case 'ArrowDown':
nextActiveChild = getNextActiveChild(1);
break;
default:
return;
}
if (!nextActiveChild) return;
event.preventDefault();
handleSelect(nextActiveChild.dataset.rbEventKey, event);
setRefocus(true);
};
useEffect(() => {
if (listNode.current && needsRefocus) {
let activeChild = listNode.current.querySelector(
'[data-rb-event-key].active',
);
if (activeChild) activeChild.focus();
}
}, [listNode, needsRefocus]);
const mergedRef = useMergedRefs(ref, listNode);
return (
<SelectableContext.Provider value={this.handleSelect}>
<NavContext.Provider value={this.state.navContext}>
<Component
{...props}
onKeyDown={this.handleKeyDown}
ref={this.attachRef}
/>
<SelectableContext.Provider value={handleSelect}>
<NavContext.Provider
value={{
role, // used by NavLink to determine it's role
activeKey: makeEventKey(activeKey),
getControlledId: getControlledId || noop,
getControllerId: getControllerId || noop,
}}
>
<Component {...props} onKeyDown={handleKeyDown} ref={mergedRef} />
</NavContext.Provider>
</SelectableContext.Provider>
);
}
}
export default mapContextToProps(
[SelectableContext, TabContext],
(parentOnSelect, tabContext, { role }) => {
if (!tabContext) return { parentOnSelect };
const { activeKey, getControllerId, getControlledId } = tabContext;
return {
activeKey,
parentOnSelect,
role: role || 'tablist',
// pass these two through to avoid having to listen to
// both Tab and Nav contexts in NavLink
getControllerId,
getControlledId,
};
},
AbstractNav,
);
AbstractNav.propTypes = propTypes;
AbstractNav.defaultProps = defaultProps;
export default AbstractNav;
......@@ -217,7 +217,7 @@ describe('<Nav>', () => {
</Nav>,
);
wrapper.assertSingle('div[role="tablist"]');
wrapper.assertSingle('.nav[role="tablist"]');
wrapper.find('a[role="tab"]').length.should.equal(2);
});
});
......
......@@ -87,11 +87,7 @@ describe('<TabContainer>', () => {
</TabContainer>,
);
instance
.find('Nav')
.getDOMNode()
.getAttribute('role')
.should.equal('tablist');
instance.assertSingle('.nav[role="tablist"]');
instance
.find('NavLink a')
......@@ -116,11 +112,7 @@ describe('<TabContainer>', () => {
</TabContainer>,
);
instance
.find('Nav')
.getDOMNode()
.getAttribute('role')
.should.equal('navigation');
instance.assertSingle('.nav[role="navigation"]');
// make sure its not passed to the Nav.Link
expect(
......
......@@ -913,10 +913,10 @@
resolved "https://registry.yarnpkg.com/@restart/context/-/context-2.1.4.tgz#a99d87c299a34c28bd85bb489cb07bfd23149c02"
integrity sha512-INJYZQJP7g+IoDUh/475NlGiTeMfwTXUEr3tmRneckHIxNolGOW9CTq83S8cxq0CgJwwcMzMJFchxvlwe7Rk8Q==
"@restart/hooks@^0.3.0":
version "0.3.9"
resolved "https://registry.yarnpkg.com/@restart/hooks/-/hooks-0.3.9.tgz#b2849bee3faec1d3fc531fdf301f4b27649baa8e"
integrity sha512-6gL84qcdZHUN0V5czyMXzAbcBBpHm8H5Gwj7eqnVi6p3JzGwJ5m2at19dKE9Gv3SsU3Hv9SPDX+6zQSExCjjkQ==
"@restart/hooks@^0.3.11":
version "0.3.11"
resolved "https://registry.yarnpkg.com/@restart/hooks/-/hooks-0.3.11.tgz#79ef8b872710838fac8c540f499915ed97935a7a"
integrity sha512-EcKbmMEMJBGNXl2u5WLpwmQnH47+USJW4wIQlwkStmsdt/igq+EUJX7tp0fha0GGIliJoxg69FSe06i5/B6HpA==
"@samverschueren/stream-to-observable@^0.3.0":
version "0.3.0"
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment