Contextual Menu Item - Passing Extra Information For MenuItem OnRender

[chang47]

Description of changes

When we're rendering other types of menu item in our ContextualMenu we expose certain information like the current index or the total number of items in a contextual menu.

We currently don't expose these values to our Menu Items when we let them use their own custom onRender(). As a result we miss information that we might need.

The specific scenario for this is when we need to set the aria-setsize and aria-posinset for custom onRender() for the swatchcolorpicker and fullgallery menu items in the contextual menu but we don't have the information to set these values correctly.

[joschect]

Once you add these in here, it's possible for the user to override them. Should they be allowed to override them? I feel like ContextualMenuItem has gotten too large and should be split. ContextualMenuItem is the information that gets passed into the contextmenu, then something like ContextualMenuItemProps are the props that will get passed to the component that renders the item.

[joschect]

@dzearing any ideas?

[chang47]

EDIT:
Oops, just saw the previous comment. Disregard what I said here.

@joschect From my understanding, even if I didn't add those values in, wouldn't we still have the same problem, because of the any property field that already exists?

If that's the case, I think the only difference is whether or not we explicitly or implicitly inform the user that we have this information, or at least in this specific instance.

[jspurlin]

I agree that the ContextualMenuItem is getting rather large, but I also think we can leverage the [propertyName: string]: any prop on IContextualMenuItem. The user could pass 'aria-posinset': <#> and 'aria-setsize': <#> to menu items already and we should piggy back off of that when passing to onRender. This would allow to do everything we need without adding any new props

[jspurlin]

If we went this route, the only thing that would need to be thought is if we want to allow creators to be able to override these values for non custom renders... Currently if a user supplies those props they will be overridden by the base render functions. If we want to allow this value to be overridden, we could do the spread (talked about in other comments) and pass the updated item to all render fuctions

[joschect]

If we do proceed with this we will need to allow users to provide their own values for this. So you should do an expand like const itemWithProps = {... {positionInSet: focusableElementIndex, setSize: totalItemCount, hasCheckMarks: hasCheckMarks, hasIcon: hasIcons}, ...item} that way their passed in item values will override the ones you provide.

[jspurlin]

I agree that we should just spread the props, assign the value and allow for it to be overridden. You wouldn't need to put the other props inside of their own object though, it could look something like:
item.onRender( {posInSet: focusableElementIndex, setSize: totalItemCount, ...item}, this.dismiss)

[dzearing]

I may not be understanding correctly, but it seems like we're mutating the item, which is a terrible idea. We shouldn't pollute the consumer input like this. having some additional props in the onRender stuff seems reasonable though.

[chang47]

Perfect, I switched to use the spread operator.

[jspurlin]

Do we currently have a need have a need for the hasCheckmarks or hasIcons? If not, it should only be added once it is needed

[chang47]

No we don't. They have been removed.

[jspurlin]

We should be leveraging the [propertyName: string]: any prop from IContextualMenuItem

[jspurlin]

Did you mean for each key to be defined differently? (e.g. on is is string and the other is a string array)

[chang47]

Whoa, I actually don't know how those braces actually made it in there. They should not be there. The intention is to pass it in as an ordinary native attribute. Sorry about that.