MenuItem supports HTML props, remove submenu prop #2087
Conversation
- add labelElement because label doesn't support JSX
fix CopyCellsMenuItem usagePreview: documentation | landing | table |
| @@ -22,10 +22,32 @@ export interface IMenuItemProps extends IActionProps, ILinkProps { | |||
| /** Item text, required for usability. */ | |||
| text: string; | |||
|
|
|||
| /** | |||
| * Children of this component will be rendered in a __submenu__ that appears when hovering or | |||
| * clicking on this menu item. You can instead pass an array of props objects to the `submenu` prop | |||
|
|
||
| /** | ||
| * Whether this menu item is non-interactive. Enabling this prop will ignore `href`, `tabIndex`, | ||
| * and mouse event handlers (in particular click, down, enter, leave) |
There was a problem hiding this comment.
missing a trailing period
| @@ -148,3 +193,13 @@ const SUBMENU_POPOVER_MODIFIERS: Popper.Modifiers = { | |||
| offset: { offset: -5 }, | |||
| preventOverflow: { boundariesElement: "viewport", padding: 20 }, | |||
| }; | |||
|
|
|||
| // props to ignore when disabled | |||
| const DISABLED_PROPS: React.AnchorHTMLAttributes<HTMLAnchorElement> = { | |||
There was a problem hiding this comment.
nit: place block-scoped variables above their usage sites
There was a problem hiding this comment.
what does this mean? where can i move this constant? it's defined outside the class so it'll actually be static, and used once in render().
There was a problem hiding this comment.
what i thought we were in the habit of putting statics at the bottom?
There was a problem hiding this comment.
feels weird for const, more acceptable for function since those get hoisted
There was a problem hiding this comment.
hoisted? both feel precisely the same level of weird to me.
There was a problem hiding this comment.
There was a problem hiding this comment.
const gets hoisted too: functions and variables.
and it's used inside a function (not synchronously), so guaranteed to be defined.
trailing periods.Preview: documentation | landing | table |
| @@ -22,10 +22,32 @@ export interface IMenuItemProps extends IActionProps, ILinkProps { | |||
| /** Item text, required for usability. */ | |||
| text: string; | |||
|
|
|||
| /** | |||
| * Children of this component will be rendered in a __submenu__ that appears when hovering or | |||
| * clicking on this menu item. You can instead pass an array of props objects to the `submenu` prop. | |||
There was a problem hiding this comment.
I still don't understand this second sentence. Instead of what?
There was a problem hiding this comment.
submenu array instead of children elements. how can i clarify?
There was a problem hiding this comment.
Do you mean to say that a user can render a submenu with either children or the submenu prop? (the "instead" wording works counter to this, it suggests that only one option works)
Why do we want to support that?
There was a problem hiding this comment.
yes that's what I mean to say. this has been supported forever.
remember when I asked about making MenuItem children into the content (text) of the item and relegating submenu to props only?
There was a problem hiding this comment.
🔥 proposal: submenu?: React.ReactNode; submenuProps?: IMenuItemProps[]
There was a problem hiding this comment.
I'd rather not break the API here and only support one usage pattern for submenus
There was a problem hiding this comment.
one usage pattern
but we already support two patterns, and have for a very long time
not break the API
removing one usage pattern would be an API break...
what should I do here?
There was a problem hiding this comment.
a cursory search suggests no one is using the submenu prop, but hard to verify.
There was a problem hiding this comment.
ohh ok I thought this was adding new support. in that case I'd suggest removing the submenu prop while we can in 2.0
giladgray
changed the title
| { icon: "align-center", text: "Align Center" }, | ||
| { icon: "align-right", text: "Align Right" }, | ||
| ]; | ||
| const wrapper = shallow(<MenuItem icon="align-left" text="Alignment" submenu={items} />); |
| ); | ||
| const submenu = findSubmenu(wrapper); | ||
| assert.lengthOf(submenu.props.children, 2); | ||
| assert.isTrue(warnSpy.alwaysCalledWith(MENU_WARN_CHILDREN_SUBMENU_MUTEX)); |
only one way to submenu: remove submenu propPreview: documentation | landing | table |
fix iconName usage in tests (semantic merge)Preview: documentation | landing | table |
Fixes #605
Changes proposed in this pull request:
labelElementbecauselabeldoesn't support JSX (just likeSwitchetc)submenuprop: only one way to submenu