Skip to content

Commit

Permalink
fix(AbstractNav): allow passed in refs to be properly forwarded (#4031)
Browse files Browse the repository at this point in the history
* 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 <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
  • Loading branch information
bpas247 authored and taion committed Aug 9, 2019
1 parent 9993869 commit bda567f
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 147 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@
"@babel/runtime": "^7.4.2",
"@react-bootstrap/react-popper": "1.2.1",
"@restart/context": "^2.1.4",
"@restart/hooks": "^0.3.0",
"@restart/hooks": "^0.3.11",
"classnames": "^2.2.6",
"dom-helpers": "^3.4.0",
"invariant": "^2.2.4",
Expand Down
247 changes: 116 additions & 131 deletions src/AbstractNav.js
Original file line number Diff line number Diff line change
@@ -1,154 +1,139 @@
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;
2 changes: 1 addition & 1 deletion test/NavSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
Expand Down
12 changes: 2 additions & 10 deletions test/TabContainerSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit bda567f

Please sign in to comment.