Fix onitem click for contextual menu with anchor item #5003
Conversation
bcarthic
changed the title
| index={ 0 } | ||
| focusableElementIndex={ 0 } | ||
| totalItemCount={ 1 } | ||
| onItemClick={ itemOnClick } |
There was a problem hiding this comment.
Why do you need to set this in the test if it's not going to be used?
There was a problem hiding this comment.
Because the snapshot has it has a function, The actual behavior should be if we have onclick then we should invoke otherwise we shouldnt have any default method.
| describe('creates a normal button', () => { | ||
| let menuItem: IContextualMenuItem; | ||
| let menuClassNames: IMenuItemClassNames; | ||
| let itemOnClick: () => void; |
There was a problem hiding this comment.
If you are going to use this function, can you use jest.fn() here instead?
| aria-disabled={ isItemDisabled(item) } | ||
| style={ item.style } | ||
| onClick={ this._onItemClick } | ||
| onClick={ onItemClick ? onItemClick.bind(this, item) : undefined } |
There was a problem hiding this comment.
I don't believe that this is the correct place to make this change. The issue is that ContextualMenuAnchor doesn't get an onItemBaseClick passed to it, so the onClick is never called in this._onItemClick. I believe the correct response is change contextualmenuitemwrapper's this._onItemClick to the following.
protected _onItemClick = (ev: React.MouseEvent<HTMLElement>): void => {
const { item, onItemClickBase, onItemClick } = this.props;
if (onItemClickBase) {
onItemClickBase(item, ev, ev.currentTarget as HTMLElement);
} else if (onItemClick) {
onItemClick(item, ev);
}
}
There was a problem hiding this comment.
Since we dont pass onItemClickBase to ContextualMenuAnchor, it is best override the onItemClick here as per our suggestion
* master: ComboBox: Fix styling when navigating menu with single select (microsoft#5017) Use require.resolve logic in builds & watch .js files from node_modules (microsoft#5007) Fix onitem click for contextual menu with anchor item (microsoft#5003) CommandBar example: fix link color on website (microsoft#4975)
3 participants
Pull request checklist
$ npm run changeDescription of changes
With #4741 , the behavior of the context menu is regressed, If you have both onclick with preventdefault and href of the context menu item, href is now obeyed and onclick is never been called. This is because now _onItemClick from ContextualMenuItemWrapper is called instead from props.
With this change, the onclick is passed from props instead of baseclass.
Focus areas to test
(ContextualMenu)
Microsoft Reviewers: Open in CodeFlow