-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dropdown arrow key support #1075
Changes from 12 commits
6f42f10
8265405
16fb213
f6b4bd3
0fc9bcc
a241db0
48c6ffc
f72f1a2
e6da66f
23e81c6
66d26f9
4fabab7
e7d42dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,11 @@ const propTypes = { | |
isHovered: PropTypes.bool, | ||
/** Default hyperlink location */ | ||
href: PropTypes.string, | ||
index: PropTypes.number, | ||
context: PropTypes.shape({ | ||
keyHandler: PropTypes.func, | ||
sendRef: PropTypes.func | ||
}), | ||
/** Additional props are spread to the container component */ | ||
'': PropTypes.any, | ||
/** Callback for click event */ | ||
|
@@ -31,31 +36,61 @@ const defaultProps = { | |
component: 'a', | ||
isDisabled: false, | ||
href: '#', | ||
onClick: Function.prototype | ||
onClick: Function.prototype, | ||
index: -1, | ||
context: { | ||
keyHandler: Function.prototype, | ||
sendRef: Function.prototype | ||
} | ||
}; | ||
|
||
class DropdownItem extends React.Component { | ||
componentWillMount() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
this.ref = React.createRef(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for this to be in the constructor. The class properties plugin is being used. This will match other components. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was able to remove the onKeyDown binding but I'm not sure where the ref should be created if outside of the constructor. Removing this breaks the ref. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is a babel plugin that pretty much abstracts away any use of the constructor & adds any definition within the class as a class property instead of having to explicitly do so in the constructor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see, cool. I removed the constructor. |
||
} | ||
|
||
componentDidMount() { | ||
this.props.context.sendRef(this.props.index, this.ref, this.props.isDisabled); | ||
} | ||
|
||
onKeyDown = event => { | ||
// Detected key press on this item, notify the menu parent so that the appropriate | ||
// item can be focused | ||
if (event.key === 'Tab') return; | ||
event.preventDefault(); | ||
if (event.key === 'ArrowUp') { | ||
this.props.context.keyHandler(this.props.index, 'up'); | ||
} else if (event.key === 'ArrowDown') { | ||
this.props.context.keyHandler(this.props.index, 'down'); | ||
} else if (event.key === 'Enter') { | ||
this.ref.click(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This, i beleive should be |
||
} | ||
}; | ||
|
||
render() { | ||
const { | ||
className, | ||
children, | ||
isHovered, | ||
context, | ||
onClick, | ||
component: Component, | ||
isDisabled, | ||
...additionalProps | ||
index, | ||
...props | ||
} = this.props; | ||
const additionalProps = props; | ||
if (Component === 'a') { | ||
additionalProps['aria-disabled'] = isDisabled; | ||
additionalProps.tabIndex = isDisabled ? -1 : additionalProps.tabIndex; | ||
} else if (Component === 'button') { | ||
additionalProps.disabled = isDisabled; | ||
additionalProps.type = additionalProps.type || 'button'; | ||
} | ||
|
||
return ( | ||
<DropdownContext.Consumer> | ||
{onSelect => ( | ||
<li> | ||
<li role="none"> | ||
{React.isValidElement(children) ? ( | ||
React.Children.map(children, child => | ||
React.cloneElement(child, { | ||
|
@@ -70,6 +105,8 @@ class DropdownItem extends React.Component { | |
<Component | ||
{...additionalProps} | ||
className={css(isDisabled && styles.modifiers.disabled, isHovered && styles.modifiers.hover, className)} | ||
ref={ref => (this.ref = ref)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since tyou are already using |
||
onKeyDown={this.onKeyDown} | ||
onClick={event => { | ||
if (!isDisabled) { | ||
if (Component === 'button') onClick && onClick(event); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,7 @@ import styles from '@patternfly/patternfly-next/components/Dropdown/dropdown.css | |
import { css } from '@patternfly/react-styles'; | ||
import PropTypes from 'prop-types'; | ||
import { componentShape } from '../../internal/componentShape'; | ||
import { DropdownPosition, DropdownContext } from './dropdownConstants'; | ||
import FocusTrap from 'focus-trap-react'; | ||
import { DropdownPosition, DropdownContext, DropdownArrowContext } from './dropdownConstants'; | ||
|
||
const propTypes = { | ||
/** Anything which can be rendered as dropdown items */ | ||
|
@@ -29,12 +28,117 @@ const defaultProps = { | |
component: 'ul' | ||
}; | ||
|
||
const DropdownMenu = ({ className, isOpen, position, children, component: Component, ...props }) => { | ||
let menu = null; | ||
if (Component === 'div') { | ||
menu = ( | ||
<DropdownContext.Consumer> | ||
{onSelect => ( | ||
class DropdownMenu extends React.Component { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to keep consistent with the repo modify this to be class DropdownMenu extends React.Component {
refsCollection = {};
keyHandler = (index, position, custom = false) => { ... }
} |
||
constructor(props) { | ||
super(props); | ||
this.refsCollection = {}; | ||
this.keyHandler = this.keyHandler.bind(this); | ||
} | ||
|
||
keyHandler(index, position, custom = false) { | ||
const kids = this.props.children; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could break if consumers put items in a fragment. It might be better to make use of context to pass things around. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you mean by using context? I am seeing that having a child that is a React.Fragment will throw an error but should we just handle that when the children get extended (wrapping them in a div) or is there a better method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The context API https://reactjs.org/docs/context.html There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can move keyHandler and sendRef to a Context but they only get passed down one layer so I'm not sure it saves much. I'm also confused about the case of putting the items in a Fragment because the top level Dropdown component should be expecting an array. |
||
if (!Array.isArray(kids)) return; | ||
let nextIndex; | ||
if (position === 'up') { | ||
if (index === 0) { | ||
// loop back to end | ||
nextIndex = kids.length - 1; | ||
} else { | ||
nextIndex = index - 1; | ||
} | ||
} else if (index === kids.length - 1) { | ||
// loop back to beginning | ||
nextIndex = 0; | ||
} else { | ||
nextIndex = index + 1; | ||
} | ||
if (this.refsCollection[`item-${nextIndex}`] === null) { | ||
this.keyHandler(nextIndex, position, custom); | ||
} else { | ||
custom | ||
? this.refsCollection[`item-${nextIndex}`].current.focus() | ||
: this.refsCollection[`item-${nextIndex}`].focus(); | ||
} | ||
} | ||
|
||
componentDidMount() { | ||
if (this.props.openedOnEnter) { | ||
if (this.props.component === 'ul') this.refsCollection['item-0'].focus(); | ||
else { | ||
this.refsCollection['item-0'].current.focus(); | ||
} | ||
} | ||
} | ||
|
||
sendRef = (index, node, isDisabled) => { | ||
if (isDisabled || node.getAttribute('role') === 'separator') { | ||
this.refsCollection[`item-${index}`] = null; | ||
} else { | ||
this.refsCollection[`item-${index}`] = node; | ||
} | ||
}; | ||
|
||
extendChildren() { | ||
return React.Children.map(this.props.children, (child, index) => | ||
React.cloneElement(child, { | ||
index | ||
}) | ||
); | ||
} | ||
|
||
extendCustomChildren() { | ||
const mappedChildren = React.Children.map(this.props.children, (child, index) => { | ||
const mappedChild = React.cloneElement(child, { | ||
ref: React.createRef(), | ||
tabIndex: -1, | ||
onKeyDown: event => { | ||
if (event.key === 'Tab') return; | ||
event.preventDefault(); | ||
if (event.key === 'ArrowUp') { | ||
this.keyHandler(index, 'up', true); | ||
} else if (event.key === 'ArrowDown') { | ||
this.keyHandler(index, 'down', true); | ||
} | ||
} | ||
}); | ||
!mappedChild.props.disabled | ||
? (this.refsCollection[`item-${index}`] = mappedChild.ref) | ||
: (this.refsCollection[`item-${index}`] = null); | ||
return mappedChild; | ||
}); | ||
return mappedChildren; | ||
} | ||
|
||
render() { | ||
const { className, isOpen, position, children, component: Component, openedOnEnter, ...props } = this.props; | ||
let menu = null; | ||
if (Component === 'div') { | ||
menu = ( | ||
<DropdownContext.Consumer> | ||
{onSelect => ( | ||
<Component | ||
{...props} | ||
className={css( | ||
styles.dropdownMenu, | ||
position === DropdownPosition.right && styles.modifiers.alignRight, | ||
className | ||
)} | ||
hidden={!isOpen} | ||
onClick={event => onSelect && onSelect(event)} | ||
> | ||
{this.extendCustomChildren()} | ||
</Component> | ||
)} | ||
</DropdownContext.Consumer> | ||
); | ||
} else if (Component === 'ul') { | ||
menu = ( | ||
<DropdownArrowContext.Provider | ||
value={{ | ||
keyHandler: this.keyHandler, | ||
sendRef: this.sendRef | ||
}} | ||
> | ||
<Component | ||
{...props} | ||
className={css( | ||
|
@@ -43,36 +147,16 @@ const DropdownMenu = ({ className, isOpen, position, children, component: Compon | |
className | ||
)} | ||
hidden={!isOpen} | ||
onClick={event => onSelect && onSelect(event)} | ||
role="menu" | ||
> | ||
{children} | ||
{this.extendChildren()} | ||
</Component> | ||
)} | ||
</DropdownContext.Consumer> | ||
); | ||
} else if (Component === 'ul') { | ||
menu = ( | ||
<FocusTrap | ||
focusTrapOptions={{ | ||
clickOutsideDeactivates: true | ||
}} | ||
> | ||
<Component | ||
{...props} | ||
className={css( | ||
styles.dropdownMenu, | ||
position === DropdownPosition.right && styles.modifiers.alignRight, | ||
className | ||
)} | ||
hidden={!isOpen} | ||
> | ||
{children} | ||
</Component> | ||
</FocusTrap> | ||
); | ||
</DropdownArrowContext.Provider> | ||
); | ||
} | ||
return menu; | ||
} | ||
return menu; | ||
}; | ||
} | ||
|
||
DropdownMenu.propTypes = propTypes; | ||
DropdownMenu.defaultProps = defaultProps; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are application launcher tests that run similar tests. Will most likely need some updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the tests.