SplitButton: When in a menu allow the primary portion to collapse submenu#4517
Conversation
…nu) to collapse a submenu
| iconProps: item.iconProps, | ||
| onMouseEnter: this._onItemMouseEnter.bind(this, item, true /* isSplitButtonPrimary */), | ||
| onMouseLeave: this._onMouseItemLeave.bind(this, item), | ||
| onMouseMove: this._onItemMouseMove.bind(this, item, true /* isSplitButtonPrimary */), |
There was a problem hiding this comment.
.bind [](start = 40, length = 5)
Question a bit unrelated to the CR, do we not prefer to tag the methods with @autoBind instead of using .bind, this way we dont have to remember to do it? I ask because I think I have seen both ways in the project and wondering if there is a preference
There was a problem hiding this comment.
No, the usage of autobind has been removed from fabric because of the bloat that came with it. Ideally you want to use functions as variables instead, but in this case since we are passing extra data to the callback we need to use bind
| checked: item.checked, | ||
| icon: item.icon, | ||
| iconProps: item.iconProps, | ||
| onMouseEnter: this._onItemMouseEnter.bind(this, item, true /* isSplitButtonPrimary */), |
There was a problem hiding this comment.
It seems like we should be treating the two buttons as more separate than they are. SplitButtonPrimary doesn't have submenu items, and shouldn't be treated as such. Instead of adding this, instead do {...item, subMenuProps: null, items: null} in render split button. It keeps it cleaner. You could also parse this further since the icon part of the split button really shouldn't get passed the onclick either.
…s not on the expandable portion of the control.
| // Delay updating expanding/dismissing the submenu | ||
| // and only set focus if we have not already done so | ||
| // and only set focus if we have not already done so. | ||
| // Also if we are on the primary portion of a splitButton |
There was a problem hiding this comment.
Remove mention of splitbutton
| // (and we know from above that we are not the expanded item) | ||
| // collapse the submenu | ||
| if (hasSubmenu(item)) { | ||
| ev.stopPropagation(); |
There was a problem hiding this comment.
Why does this need to be here now?
There was a problem hiding this comment.
I bumped the hover handling on to the whole container of the splitButton since that's what should be getting focus. This was added to make sure when we are hovering over the expandable portion of the splitButton that the event will not bubble up to the parent container. All other cases it doesn't change anything. This PR now also fixes focus not updating correctly when hovering over a splitButton (in a menu), now splitButtons update focus just like the other items
4 participants
SplitButton: Allow the primary portion of a splitButton (when it a menu) to collapse a submenu
Pull request checklist
$ npm run changeDescription of changes
Before this PR, the primary portion of a splitButton when in a menu would not collapse another item's submenu (you could only hover over the split portion of the splitButton). This did not align with the behavior of other menu items and felt weird/broken
Focus areas to test
Verified that now the primary portion of splitButtons can collapse submenus (if it is not its own item's submenu) on hover just like all other menu items AND no other behavior changed