Skip to content

Conversation

@KevinTCoughlin
Copy link
Member

Pull request checklist

Description of changes

When a CommandBar's item had an href set we would create an <a> with an onClick bound to that item's onClick handler, rather than invoking the onClick delegate in CommandBar. Because of this onClick was invoked without the backing item for this scenario compared with not having an href set. This change attempts to normalize the click handling by invoking CommandBar's onClick handler which in turn invokes the item's onClick handler.

Focus areas to test

  1. Set "Persist Logs" to true in your console or set a break-point at https://github.com/OfficeDev/office-ui-fabric-react/blob/master/packages/office-ui-fabric-react/src/components/CommandBar/CommandBar.tsx#L437
  2. If you're checking logs, put a print statement in the onClick callback for the CommandBar example here: https://github.com/OfficeDev/office-ui-fabric-react/blob/master/packages/office-ui-fabric-react/src/components/CommandBar/examples/data-nonFocusable.ts#L25
  3. Also set an href on that item, for example { ... href: 'https://www.bing.com/ }
  4. Run the solution locally and navigate to http://localhost:4322/#/examples/commandbar. Click on both items with and without href set.
    • If the href IS NOT set the behavior should be as it is on master.
    • If the href IS set, then the item's onClick handler should be invoked with the DOM event and backing item before handling the href. Previously item was undefined.

I feel like this change requires at least an additional test if we wish to merge it so I welcome any guidance there. Will look at what exists in the meantime.

Copy link
Member

@micahgodbolt micahgodbolt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normalizing click behavior is always a good thing.

@manishgarg1 manishgarg1 merged commit 9e838b8 into microsoft:master Apr 17, 2018
@KevinTCoughlin KevinTCoughlin deleted the keco/fix-href-commandbar-item branch April 18, 2018 16:08
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CommandBar Items with href passes undefined item in onClick handler

3 participants