feat(Menu): Add wrapper prop to MenuItem #323
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import * as React from 'react' | |
|
|
||
| import { childrenExist, createShorthandFactory, customPropTypes, UIComponent } from '../../lib' | ||
| import Icon from '../Icon/Icon' | ||
| import Slot from '../Slot/Slot' | ||
| import { menuItemBehavior } from '../../lib/accessibility' | ||
| import { Accessibility, AccessibilityActionHandlers } from '../../lib/accessibility/types' | ||
| import IsFromKeyboard from '../../lib/isFromKeyboard' | ||
|
|
@@ -81,6 +82,15 @@ export interface MenuItemProps | |
| */ | ||
| renderIcon?: ShorthandRenderFunction | ||
|
|
||
| /** | ||
| * A custom render function the wrapper slot. | ||
| * | ||
| * @param {React.ReactType} Component - The computed component for this slot. | ||
| * @param {object} props - The computed props for this slot. | ||
| * @param {ReactNode|ReactNodeArray} children - The computed children for this slot. | ||
| */ | ||
| renderWrapper?: ShorthandRenderFunction | ||
|
|
||
| /** The menu item can have secondary type. */ | ||
| secondary?: boolean | ||
|
|
||
|
|
@@ -89,6 +99,9 @@ export interface MenuItemProps | |
|
|
||
| /** A vertical menu displays elements vertically. */ | ||
| vertical?: boolean | ||
|
|
||
| /** Shorthand for the wrapper component. */ | ||
| wrapper?: ShorthandValue | ||
| } | ||
|
|
||
| export interface MenuItemState { | ||
|
|
@@ -123,46 +136,56 @@ class MenuItem extends UIComponent<Extendable<MenuItemProps>, MenuItemState> { | |
| underlined: PropTypes.bool, | ||
| vertical: PropTypes.bool, | ||
| renderIcon: PropTypes.func, | ||
| wrapper: PropTypes.oneOfType([PropTypes.node, PropTypes.object]), | ||
| renderWrapper: PropTypes.func, | ||
| } | ||
|
|
||
| static defaultProps = { | ||
| as: 'a', | ||
| accessibility: menuItemBehavior as Accessibility, | ||
| wrapper: { as: 'li' }, | ||
bmdalex marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| state = IsFromKeyboard.initial | ||
|
|
||
| renderComponent({ ElementType, classes, accessibility, rest }) { | ||
| const { children, content, icon, renderIcon, renderWrapper, wrapper } = this.props | ||
|
|
||
| const menuItemInner = childrenExist(children) ? ( | ||
| children | ||
| ) : ( | ||
| <ElementType | ||
| className={classes.root} | ||
|
Collaborator
There was a problem hiding this comment. when we discussed changes for |
||
| onClick={this.handleClick} | ||
| onBlur={this.handleBlur} | ||
| onFocus={this.handleFocus} | ||
| {...accessibility.attributes.anchor} | ||
| {...accessibility.keyHandlers.anchor} | ||
| {...rest} | ||
| > | ||
| {icon && | ||
| Icon.create(this.props.icon, { | ||
| defaultProps: { xSpacing: !!content ? 'after' : 'none' }, | ||
| render: renderIcon, | ||
| })} | ||
| {content} | ||
| </ElementType> | ||
| ) | ||
|
|
||
| if (wrapper) { | ||
| return Slot.create(wrapper, { | ||
| defaultProps: { | ||
| className: cx('ui-menu__item__wrapper', classes.wrapper), | ||
| ...accessibility.attributes.root, | ||
| ...accessibility.keyHandlers.root, | ||
| }, | ||
| render: renderWrapper, | ||
| overrideProps: () => ({ | ||
| children: menuItemInner, | ||
|
Collaborator
There was a problem hiding this comment. very nice usage of
Member
There was a problem hiding this comment. In this case we can use |
||
| }), | ||
| }) | ||
| } | ||
| return menuItemInner | ||
| } | ||
|
|
||
| protected actionHandlers: AccessibilityActionHandlers = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ export interface Conformant { | |
| requiredProps?: object | ||
| exportedAtTopLevel?: boolean | ||
| rendersPortal?: boolean | ||
| usesWrapperSlot?: boolean | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -32,13 +33,15 @@ export interface Conformant { | |
| * @param {boolean} [options.exportedAtTopLevel=false] Is this component exported as top level API? | ||
| * @param {boolean} [options.rendersPortal=false] Does this component render a Portal powered component? | ||
| * @param {Object} [options.requiredProps={}] Props required to render Component without errors or warnings. | ||
| * @param {boolean} [options.usesWrapperSlot=false] This component uses wrapper slot to wrap the 'meaningful' element. | ||
| */ | ||
| export default (Component, options: Conformant = {}) => { | ||
| const { | ||
| eventTargets = {}, | ||
| exportedAtTopLevel = true, | ||
| requiredProps = {}, | ||
| rendersPortal = false, | ||
| usesWrapperSlot = false, | ||
miroslavstastny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } = options | ||
| const { throwError } = helpers('isConformant', Component) | ||
|
|
||
|
|
@@ -58,6 +61,14 @@ export default (Component, options: Conformant = {}) => { | |
| component = component.childAt(0) // skip the additional wrap <div> of the FocusZone | ||
| } | ||
| } | ||
|
|
||
| if (usesWrapperSlot) { | ||
|
Collaborator
There was a problem hiding this comment. @kuzhelov @levithomason @miroslavstastny AFAIR, the tests that fail are related to This probably requires a session where we can discuss options in more detail; @miroslavstastny feel free to schedule a meeting with me and @kuzhelov
Member
Author
There was a problem hiding this comment. Thank you very much for the review, @Bugaa92. So the main question is whether we should split props between I am not strictly against doing the split, but when I was considering this I thought it would make more sense to do 'everything to We should consider usability and API consistency first and test after that. Agree to discuss this offline.
Contributor
There was a problem hiding this comment. thanks for sharing your thoughts, @miroslavstastny, @Bugaa92
If I would answer this question only for the MenuItem component I would rather choose to not split and apply all the props to wrapped <MenuItem styles={..} href={..} />It would be really confusing and non-intuitive to me if At the same time, agree with the @Bugaa92's reasons for the concerns he has expressed, although see Agree to strive for consistent and intuitive API, and for this sake, let's, please, schedule a meeting as you've proposed - I will be back staring from 7th of November, but let me know if it should be decided earlier, will do all my best to allocate time for that. Thank you! |
||
| component = component | ||
| .childAt(0) | ||
| .childAt(0) | ||
| .childAt(0) | ||
| } | ||
|
|
||
| return component | ||
| } | ||
|
|
||
|
|
@@ -396,7 +407,7 @@ export default (Component, options: Conformant = {}) => { | |
| .find('[className]') | ||
| .hostNodes() | ||
| .filterWhere(c => !c.prop(FOCUSZONE_WRAP_ATTRIBUTE)) // filter out FocusZone wrap <div> | ||
| .at(usesWrapperSlot ? 1 : 0) | ||
| .prop('className') | ||
| return classes | ||
| } | ||
|
|
||
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.
when we discussed changes for
Inputcomponent we had a consensus thatasandstylesprops always go to the root element; we might want the same forMenuItem; let's see what others say @levithomason @kuzhelovThere 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.
IIRC there is an accessibility requirement that the click listener needs to be on the anchor tag. The primary use case for
ason the menu item is to allow adding router functionality. This means the router's navigation handler needs to be applied to the anchor:If the
asprop goes to the root, it will still work as the event will bubble from the anchor up to theli, however, it would not be the correct markup and behavior for the accessibility requirement.