Sub-component style support in mergeStyleSets #5513

cliffkoh · 2018-07-11T01:13:25Z

Pull request checklist

Addresses an existing issue: Fixes MergeStyles: Support StyleFunctions #5370
Include a change request file using $ npm run change

Description of changes

Support passing in a stylefunction or object to subcomponent styles (in a new way that doesn't require conditional types).
Substantially improve typings which results in way better intellisense, and more errors being caught.

Very rough example of what you can do now in a .styles.ts

export const getStyles = {
        root: { background: 'red' },
        a: { background: 'green' },
        subComponentStyles: {
          label: (props: ILabelStyleProps) => ({
             root: { background: 'green', fontSize: 12 }
          });
        }
      };

A user calling the component can then pass in:

<SomeComponent styles={{
        a: { background: 'white' },
        b: { background: 'blue' },
        subComponentStyles: {
          label:  {  // you can also pass in objects
            root: {
               background: 'yellow',
               color: 'pink'
            }
          }
        }
      }} />

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

cliffkoh · 2018-07-11T01:43:16Z

packages/utilities/src/IClassNames.ts

@@ -1 +0,0 @@
-export type IClassNames<T> = { [key in keyof T]: string };


I would like to major bump @uifabric/utilities because of this change. IClassNames simply doesn't make sense now (semantically wrong, in addition to structurally wrong) because it doesn't just have classnames...

dzearing

pull master
resolve "object" error
resolve typing issue

…c-react into cliffkoh/styleFunctionsV2

cliffkoh · 2018-07-12T01:28:34Z

@dzearing pulled and investigated the new errors. Unfortunately I think there is a deeper issue with newly introduced DetailsRow code and one for which a mechanical fix wouldn't be the right fix. Sent an email to @kenotron and will need him to resolve the issue in DetailsRow first.

kenotron · 2018-07-12T04:13:05Z

packages/example-app-base/src/components/templates/PageImageSet.tsx

-      getDefaultStyles(styleProps),
-      getStyles && getStyles(styleProps)
-    );
+    const classNames = getStyles


This LOOKS cleaner - but is there something fundamentally wrong with having a null as a 2nd param?

There is a type error.. I didn't change this because I was too free 😁

example-app-base could use a lot of work to get spruced up to modern standards. In modern standards, we don't call mergeStyleSets from components but we call classNameFunction/getClassNames instead. dzearing asked me to do the bare minimum though and to invest my time in Dropdown conversion instead.

kenotron · 2018-07-12T04:30:28Z

packages/merge-styles/src/concatStyleSets.ts

+
+/**
+ * Combine a set of styles together (but does not register css classes).
+ * @param styleSet1 The first style set to be concatenated.


@param styleSet

kenotron · 2018-07-12T04:30:58Z

packages/merge-styles/src/concatStyleSets.ts

+  styleSet6: TStyleSet6 | false | null | undefined
+): IConcatenatedStyleSet<TStyleSet1 & TStyleSet2 & TStyleSet3 & TStyleSet4 & TStyleSet5 & TStyleSet6>;
+
+/**


Don't know if my eyes are playing tricks on me but is this the same as the implementation below?

Yes, it is the same.

kenotron · 2018-07-12T04:41:15Z

packages/office-ui-fabric-react/src/components/Button/examples/Button.Split.Example.tsx

@@ -14,7 +14,7 @@ export class ButtonSplitExample extends React.Component<IButtonProps> {
    const { disabled, checked } = this.props;

    const getClassNames = classNamesFunction<IButtonBasicExampleStyleProps, IButtonBasicExampleStyles>();
-    const classNames = getClassNames(getStyles);
+    const classNames = getClassNames(getStyles, {});


I don't think that {} is needed - the definition of getClassNames already defaults to {} for you...

Yes, initially I didn't allow it but David decided to keep it optional for backwards compatibility. The original version would have been undefined. My POV is that if you don't need style props, you should just pass in a style object and so all style functions should take in the 2nd arg.

kenotron · 2018-07-12T04:43:53Z

packages/merge-styles/tslint.json

@@ -1,4 +1,6 @@
 {
  "extends": ["office-ui-fabric-react-tslint"],
-  "rules": {}
+  "rules": {
+    "no-any": false


I think we could just do tslint:disable:no-any like the other places

My POV is that this library in particular has a high density of real, unavoidable uses of any (the implementation function in the overloads, for instance). Having a ton of tslint:disable scattered around is a big mess.

We still disallow implicit anys, and any use of any will be scrutinized.

kenotron · 2018-07-12T04:45:42Z

packages/utilities/src/classNamesFunction.ts

+    // However, because we usually use `props.styles` as the argument to an invocation of this method, and
+    // `props.styles` itself is defined as optional, this avoids the need to use `!` at all invocation points.
+    if (styleFunctionOrObject === undefined) {
+      // throw new TypeError(


Remove commented code

Hm I already did - must have forgotten to push 😅

kenotron · 2018-07-12T04:50:34Z

packages/merge-styles/src/IStyleSet.ts

+/**
+ * A concatenated style set differs from `IStyleSet` in that subComponentStyles will always be a style function.
+ */
+export type IConcatenatedStyleSet<TStyleSet extends IStyleSet<TStyleSet>> = {


There is no difference between this and IProcessedStyleSet... I'm thinking about whether we gain much in differentiating them. The difference is that mergestyleset has a side effect of registering them but concat doesn't. The result is the same type structurally.

There is a difference, look closely.

One contains classnames, one contains IStyles.

This is not just specified in the code, but called out in the comments as well. :)

cliffkoh · 2018-07-12T07:42:32Z

@kenotron Appreciate if you could review the changes to DetailsRow in 97e4fd4 (part of this PR) and sign off on it by approving the PR. There's technically a breaking change in #5514 which you're also doing because we're removing a prop...

@Vitalius1, do you know why there is another copy of Shimmer in experiments? Should we be de-duping?

@JasonGore appreciate if you could sign off on the createComponent changes in experiments too.

kenotron

These changes work in DetailsRow I believe.

Vitalius1 · 2018-07-12T17:53:04Z

@cliffkoh The Shimmer from experiments can not be removed as of now as it's being used in common's packages.

…-react into cliffkoh/styleFunctionsV2

…c-react into cliffkoh/styleFunctionsV2

srideshpande · 2018-07-12T21:12:11Z

Signing off from ChoiceGroup perspective. #Resolved

srideshpande

Jahnp

This is awesome!

JasonGore · 2018-07-12T22:08:58Z

packages/merge-styles/src/IStyleSet.ts

+};
+
+/**
+ * A processed style set is one which the set of styles associated with each area has been converted


JasonGore · 2018-07-12T22:13:16Z

packages/utilities/src/styled.tsx

+    // It might not be necessary once  Omit becomes part of lib.d.ts (when we remove our own Omit and rely on
+    // the official version).
+    // tslint:disable-next-line:no-any
+    return concatStyleSets(...(result as any)) as IConcatenatedStyleSet<TStyleSet>;


Could it be related to this? I feel like I had a similar issue.

* Revert "Sub-component style support in mergeStyleSets (#5513)" This reverts commit 7faceae. * Adding change files. * updating shrinkwrap.

* Initial revert * Fix forward

cliffkoh added 3 commits July 6, 2018 18:55

Merge-styles package fully building

90a13a6

Fix downstream packages

78d0cf8

Change files

67991ab

cliffkoh requested review from atneik, dzearing, Jahnp, micahgodbolt and srideshpande as code owners July 11, 2018 01:13

cliffkoh requested a review from Vitalius1 July 11, 2018 01:19

cliffkoh commented Jul 11, 2018

View reviewed changes

cliffkoh requested review from kenotron and JasonGore July 11, 2018 22:17

Revert breaking changes in utilities and fix example-app-base

fef6c17

dzearing approved these changes Jul 12, 2018

View reviewed changes

cliffkoh added 2 commits July 11, 2018 17:42

More fixes for downstream packages

19e475d

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

4cdd9cc

…c-react into cliffkoh/styleFunctionsV2

cliffkoh requested a review from kkjeer as a code owner July 12, 2018 01:25

New change files

ac7e9c6

Improve JSDoc

ef19f96

kenotron reviewed Jul 12, 2018

View reviewed changes

cliffkoh added 2 commits July 12, 2018 00:38

Fix experiments package

97e4fd4

PR feedback

b9749a0

Change file

95c3f5b

kenotron approved these changes Jul 12, 2018

View reviewed changes

cliffkoh closed this Jul 12, 2018

cliffkoh reopened this Jul 12, 2018

cliffkoh closed this Jul 12, 2018

cliffkoh reopened this Jul 12, 2018

cliffkoh closed this Jul 12, 2018

cliffkoh reopened this Jul 12, 2018

cliffkoh added 2 commits July 12, 2018 12:03

Merge branch 'master' of https://github.com/cliffkoh/office-ui-fabric…

af2135b

…-react into cliffkoh/styleFunctionsV2

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

3a85bd6

…c-react into cliffkoh/styleFunctionsV2

kkjeer approved these changes Jul 12, 2018

View reviewed changes

atneik approved these changes Jul 12, 2018

View reviewed changes

micahgodbolt approved these changes Jul 12, 2018

View reviewed changes

srideshpande approved these changes Jul 12, 2018

View reviewed changes

Jahnp approved these changes Jul 12, 2018

View reviewed changes

JasonGore approved these changes Jul 12, 2018

View reviewed changes

Merge branch 'master' into cliffkoh/styleFunctionsV2

799936f

cliffkoh merged commit 7faceae into microsoft:master Jul 13, 2018

cliffkoh deleted the cliffkoh/styleFunctionsV2 branch July 13, 2018 06:56

dzearing added a commit that referenced this pull request Jul 13, 2018

Backing out TypeScript bump. (#5558)

83486db

* Revert "Sub-component style support in mergeStyleSets (#5513)" This reverts commit 7faceae. * Adding change files. * updating shrinkwrap.

cliffkoh mentioned this pull request Jul 17, 2018

Reintroduce #5513 #5609

Merged

cliffkoh added a commit to cliffkoh/office-ui-fabric-react that referenced this pull request Jul 17, 2018

Fix forward for microsoft#5513

5c71581

dzearing pushed a commit that referenced this pull request Jul 18, 2018

Reintroduce #5513 (#5609)

9954522

* Initial revert * Fix forward

JasonGore mentioned this pull request Jul 19, 2018

TextField: Convert to JS Styling #5639

Merged

2 tasks

Nimelrian mentioned this pull request Aug 24, 2018

[merge-styles] IProcessedStyleSet<T> maps Styles interface with subComponentStyles incorrectly, rendering it virtually useless #6059

Closed

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sub-component style support in mergeStyleSets #5513

Sub-component style support in mergeStyleSets #5513

cliffkoh commented Jul 11, 2018 •

edited

Loading

cliffkoh Jul 11, 2018

dzearing left a comment

cliffkoh commented Jul 12, 2018

kenotron Jul 12, 2018

cliffkoh Jul 12, 2018

kenotron Jul 12, 2018

cliffkoh Jul 12, 2018

kenotron Jul 12, 2018

cliffkoh Jul 12, 2018

kenotron Jul 12, 2018

cliffkoh Jul 12, 2018

kenotron Jul 12, 2018

cliffkoh Jul 12, 2018

kenotron Jul 12, 2018

cliffkoh Jul 12, 2018

kenotron Jul 12, 2018

cliffkoh Jul 12, 2018

cliffkoh Jul 12, 2018

cliffkoh commented Jul 12, 2018

kenotron left a comment

Vitalius1 commented Jul 12, 2018 •

edited

Loading

srideshpande commented Jul 12, 2018 •

edited

Loading

srideshpande left a comment

Jahnp left a comment

JasonGore Jul 12, 2018

JasonGore Jul 12, 2018

		@@ -1 +0,0 @@
		export type IClassNames<T> = { [key in keyof T]: string };

Sub-component style support in mergeStyleSets #5513

Sub-component style support in mergeStyleSets #5513

Conversation

cliffkoh commented Jul 11, 2018 • edited Loading

Pull request checklist

Description of changes

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

dzearing left a comment

Choose a reason for hiding this comment

cliffkoh commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffkoh commented Jul 12, 2018

kenotron left a comment

Choose a reason for hiding this comment

Vitalius1 commented Jul 12, 2018 • edited Loading

srideshpande commented Jul 12, 2018 • edited Loading

srideshpande left a comment

Choose a reason for hiding this comment

Jahnp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffkoh commented Jul 11, 2018 •

edited

Loading

Vitalius1 commented Jul 12, 2018 •

edited

Loading

srideshpande commented Jul 12, 2018 •

edited

Loading