Fluentui: MergeStyle problems with IContextualMenuClassNames

[chang47]

Environment Information

• __Package version(s)__: 6.96.0

Please provide a reproduction of the bug in a codepen:

I see that in the PR: https://github.com/OfficeDev/office-ui-fabric-react/commit/1882ecb9c772a19ab6c75bdddc1ca539c932e9c4 we added subComponentStyles to IContextualMenuClassItems, however when I tried to update my codebase to use the more recent fabric version, I run into problems with MergeStyles when I try to return its results as a IContextualMenuClassName, with the error:

error  TS2322: Type '{ title: string; container: string; root: string; header: string; list: string; subComponentStyles: {}; }' is not assignable to type 'IContextualMenuClassNames'.
  Types of property 'subComponentStyles' are incompatible.
    Type '{}' is not assignable to type 'IContextualMenuSubComponentStyles'.
      Property 'callout' is missing in type '{}'.

I tried to add the missing style into my style that I pass into MergeStyle, but the returning type doesn't match the return type for IContextualMenuClassNames.

I had to use a work around to get my code to work where I spread the results of MergeStyle and then add the subComponentStyle afterwards:

 {...mergeStyleSets({
    root: styles.root,
    list: styles.list,
    header: styles.header,
    title: styles.title
 }),
 subComponentStyles: styles.subComponentStyles
};

I debugged through the code and I believe the reason is that mergeStyles just take whatever is inside subComponentStyles and pass it in, which might lead to the wrong type.

Actual behavior:

Here's a case of the mergeStyle where it fails:

{subComponentStyles: {…}, container: "container-245", root: "root-246", list: "list-247", header: "header-248", …}container: "container-245"
header: "header-248"
list: "list-247"
root: "root-246"
subComponentStyles:callout: ƒ (styleProps)
__proto__: Object
title: "title-249"
__proto__: Object

Expected behavior:

Here's a case where I had to manually add the className to make the code compille:

{subComponentStyles: {…}, container: "container-245", root: "root-246", list: "list-247", header: "header-248", …}container: "container-245"
header: "header-248"
list: "list-247"
root: "root-246"
subComponentStyles:callout: {}
__proto__: Object
title: "title-249"
__proto__: Object

Notice that the difference is the subComponentStyles:callout

Priorities and help requested:

Are you willing to submit a PR to fix? No

Requested priority: High

Products/sites affected: Word Online

[chang47]

@Vitalius1 @cliffkoh

[Vitalius1]

@chang47 what prop do you use to pass in the styles?

[Vitalius1]

It looks like the issue comes from IContextualMenuClassNames interface in here that I added.

[Vitalius1]

@chang47 would be nice to have a codepen repro of the way you are passing the styles to have a debug environment.

[chang47]

Here's a codepen that I made that gives an example of what I'm doing codepen

One thing I note to note is that the code actually works just fine, I think that's because in runtime it is not a problem, however in terms of type checking which codepen doesn't do, we have the problem.

You'll have to put a breakpoint on getContextualMenuClassNames() to see what gets created.

[Vitalius1]

@cliffkoh Not sure if this is an issue with mergeStyleSets or the type I used on IContextualMenuClassNames for subComponentStyles. Also, I noticed that @chang47 is still using the deprecated prop getMenuClassNames.

[Vitalius1]

https://codepen.io/vitalius1/pen/rqXWaz here is codepen using the styles prop.

[chang47]

Thanks for the suggestion, that'll be in the list of things that we need to do.

However so we're on the same page, the problem isn't because I'm using getMenuClassNames it's because when I use mergeStyles with a expected return type of IContextualMenuClassNames, the subComponentStyles is not the correct type(?)

[chang47]

It looks like I'm having similar problems with concatStyleSets

[JasonGore]

I'll assign to @Vitalius1 for initial triage based on recent changes for now. Thanks!

[cliffkoh]

I've spent some time trying to repro this and I got really confused (while still failing to get a repro). A big part of the problem is that ContextualMenu does not conform to just about every other Fabric component.

While not evident in the Codepen repro, the IContextualMenuClassNames is not even an exported interface (it is not part of our API surface and you will need a deep import of import { IContextualMenuClassNames } from 'office-ui-fabric-react/lib/components/ContextualMenu/ContextualMenu.classNames' to get to it.

FWIW though, the following code (without the workaround) works just fine for me.

I wonder if it is a TypeScript version problem... @chang47, what TypeScript version are you on?

[cliffkoh]

I've tested both on TypeScript 2.8.4 and TypeScript 3.1.3.

I highly recommend using the updated styles prop API instead (which will avoid the need to manually call mergeStyleSet or concatStyleSet).

[chang47]

Hmm okay, I'll look into migrating our code then since I'm not sure what's happening either. We're using TS 3.1.5.

Is using the styles prop as easy as just adding whatever we had before and putting it in directly?

[cliffkoh]

I don't have very deep context of ContextualMenu (pun not intended :)) but in theory it should be :)

[Vitalius1]

@chang47 in the codepen I provided it shows that you just pass the styles to the styles prop and no need of mergeStyleSets as it does everything for you. It's a lot more simpler and cleaner code.

[Vitalius1]

@chang47
We have a fix merged in master in #6936 which should fix the errors you were facing. With that said... we highly recommend updating your code using the styles prop as it makes your code much cleaner and should not require you to do many changes (consider it more as a cleanup), and it also prevents you having a surprise breaking change to fix in a hurry when we finally remove the deprecated API. Thanks for reporting the issue even if you found a workaround.

[msft-github-bot]

(Sorry for this late notification - I was struggling with technical difficulties but that should all be resolved moving forward.)

:tada:This issue was addressed in #6936, which was successfully released as [email protected].:tada:

Handy Links:

• Release Notes
• Official NPM Release
• Test the release with Developer Demo Site