Fix styling issues found in example-app-base cleanup #8467

ecraig12345 · 2019-03-25T23:32:22Z

Pull request checklist

Include a change request file using $ npm run change

Description of changes

Fixing some styling-related issues found during example-app-base cleanup:

remove CSS global utility classes defined by ExampleCard
make FeedbackList not depend on some random class names from the list ghosting example
make various places use built-in constants and functions for handling high contrast, rather than rolling their own

Focus areas to test

Test all modified components/examples in high contrast mode

Microsoft Reviewers: Open in CodeFlow

ecraig12345 · 2019-03-25T23:50:42Z

packages/example-app-base/src/components/FeedbackList/FeedbackList.styles.ts

+import { getTheme, mergeStyleSets, DefaultFontStyles, getFocusStyle } from 'office-ui-fabric-react/lib/Styling';
+
+const theme = getTheme();
+export const classNames = mergeStyleSets({


So far I'm converting only the former ms-ListGhostingExample-* classes to css-in-js. The rest of the conversions will come later.

Hmm, another option would be to replace the divs with Stacks and just use the styles prop...

Not quite sure offhand what that would look like? I'd prefer to leave this as-is for this PR and possibly come back to it later.

ecraig12345 · 2019-03-26T00:03:13Z

packages/charting/src/components/Legends/Legends.styles.ts

@@ -20,7 +21,7 @@ export const getStyles = (props: ILegendStyleProps): ILegendsStyles => {
    },
    rect: {
      selectors: {
-        '@media screen and (-ms-high-contrast: active)': {
+        [HighContrastSelector]: {


HighContrastSelector definition for reference (it's identical to the previous string)

ecraig12345 · 2019-03-26T00:04:26Z

...ui-fabric-react/src/components/Coachmark/PositioningContainer/PositioningContainer.styles.ts

+            }
+          }
+        },
+        focusClear()


focusClear definition for reference (it's functionally identical to what the custom method was doing)

ecraig12345 · 2019-03-26T00:05:26Z

packages/office-ui-fabric-react/src/components/Keytip/examples/Keytips.Basic.Example.tsx

-                Bing
-              </Link>
-            </p>
+          <PivotItem headerText="Pivot 1" keytipProps={keytipMap.Pivot1Keytip} style={pivotItemStyle}>


This usage of u-marginBottom was better handled with a Stack

ecraig12345 · 2019-03-26T00:07:07Z

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


 export interface IKeytipsButtonExampleState {
  btnDisabled: boolean;
 }

+const marginBottom = mergeStyles({ marginBottom: 28 });


Recommend enabling "Hide whitespace changes" to view this file...the actual changes are:

use a Stack rather than u-marginRight for horizontal spacing

use this button style rather than u-marginBottom

This seems to be used for styles props. Is mergeStyles needed? I feel like this instead could be:

const marginBottomStyle = { root: { marginBottom: 28 } };

and just passed directly as the styles prop.

Good point. Fixed that.

ecraig12345 · 2019-03-26T00:07:55Z

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

+            }}
+          />
+          <DefaultButton
+            styles={{ splitButtonContainer: { height: 32 } }}


And the other actual change (3) is giving the split button a height--for some reason after changing the other styles, it was filling the height of the container rather than just being the same height as its main button.

ecraig12345 · 2019-03-26T00:08:48Z

packages/office-ui-fabric-react/src/components/List/examples/List.Ghosting.Example.tsx

+    maxHeight: 500
+  },
+  itemCell: [
+    getFocusStyle(theme, -1),


getFocusStyle does almost exactly the same thing as the focus-border mixin, though with slightly different parameter defaults.

size-auditor · 2019-03-26T00:11:05Z

Bundle test	Size (minified)	Diff from master
PositioningContainer	73.93 kB	20 bytes
Coachmark	93.567 kB	20 bytes
Dropdown	205.487 kB	5 bytes
Breadcrumb	178.62 kB	-67 bytes
DocumentCard	192.02 kB	-67 bytes
GroupedList	118.812 kB	-67 bytes
Link	54.5 kB	-67 bytes
ShimmeredDetailsList	219.565 kB	-79 bytes
DetailsList	208.741 kB	-79 bytes

Exceeds Tolerance Exceeds Baseline Below Baseline 1 kB = 1000 bytes

KevinTCoughlin

-- Signing-off on List related changes, thank you so much for doing this!!

JasonGore · 2019-03-26T20:13:53Z

packages/office-ui-fabric-react/src/components/Keytip/examples/Keytips.Basic.Example.tsx

-              </Link>
-            </p>
+          <PivotItem headerText="Pivot 1" keytipProps={keytipMap.Pivot1Keytip} style={pivotItemStyle}>
+            <Stack gap={20}>


Just an FYI mainly, #8247 adds childrenGap as added as a token replacing the gap prop. This means gap={20} sprinkled through this file could just be a centralized token object similar to styles objects.

Good to know, though I'd like to go ahead and check this in rather than waiting on another PR (since I have multiple other changes waiting on this one)

JasonGore · 2019-03-26T20:15:23Z

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


 export interface IKeytipsButtonExampleState {
  btnDisabled: boolean;
 }

+const marginBottom = mergeStyles({ marginBottom: 28 });


This seems to be used for styles props. Is mergeStyles needed? I feel like this instead could be:

const marginBottomStyle = { root: { marginBottom: 28 } };

and just passed directly as the styles prop.

packages/office-ui-fabric-react/src/components/Keytip/examples/Keytips.Basic.Example.tsx

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

khmakoto

LGTM, thanks for doing this work!

khmakoto · 2019-03-26T22:04:18Z

@ecraig12345 it appears you need to update the API of the merge-styles package.

Edit: This will be done by #8477.

ecraig12345 · 2019-03-26T22:12:12Z

@khmakoto That's actually due to (essentially) a non-obvious merge conflict from Natalie's api-extractor PR. It's being fixed in #8477.

khmakoto · 2019-03-26T22:13:36Z

@ecraig12345 just edited my comment to point that out. But thank you for the heads up!

ecraig12345 · 2019-03-27T00:48:40Z

Updated snapshots to fix build failure.

msft-github-bot · 2019-03-27T12:37:40Z

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

msft-github-bot · 2019-03-27T12:37:41Z

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

msft-github-bot · 2019-03-27T12:37:45Z

🎉[email protected] has been released which incorporates this pull request.:tada:

Handy links:

ecraig12345 requested a review from kenotron March 25, 2019 23:32

ecraig12345 requested review from JasonGore, KevinTCoughlin, khmakoto and Raghurk as code owners March 25, 2019 23:32

msft-github-bot assigned khmakoto Mar 25, 2019

ecraig12345 force-pushed the exampleapp-related-cleanup branch from a17f7af to 5002809 Compare March 25, 2019 23:46

ecraig12345 commented Mar 25, 2019

View reviewed changes

ecraig12345 commented Mar 26, 2019

View reviewed changes

ecraig12345 force-pushed the exampleapp-related-cleanup branch 2 times, most recently from 85d7965 to 5700f13 Compare March 26, 2019 01:31

KevinTCoughlin approved these changes Mar 26, 2019

View reviewed changes

JasonGore reviewed Mar 26, 2019

View reviewed changes

khmakoto reviewed Mar 26, 2019

View reviewed changes

packages/office-ui-fabric-react/src/components/Keytip/examples/Keytips.Basic.Example.tsx Show resolved Hide resolved

packages/office-ui-fabric-react/src/components/Keytip/examples/Keytips.Button.Example.tsx Outdated Show resolved Hide resolved

khmakoto approved these changes Mar 26, 2019

View reviewed changes

ecraig12345 force-pushed the exampleapp-related-cleanup branch from d3da59b to cc6514a Compare March 26, 2019 21:57

ecraig12345 force-pushed the exampleapp-related-cleanup branch from cc6514a to 5d72d73 Compare March 26, 2019 22:25

Fix styling issues found in example-app-base cleanup

07ad609

ecraig12345 force-pushed the exampleapp-related-cleanup branch from 5d72d73 to 07ad609 Compare March 27, 2019 00:47

dzearing merged commit bbb924c into microsoft:master Mar 27, 2019

ecraig12345 deleted the exampleapp-related-cleanup branch March 27, 2019 07:17

dzearing approved these changes Apr 4, 2019

View reviewed changes

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

Fix styling issues found in example-app-base cleanup #8467

Fix styling issues found in example-app-base cleanup #8467

ecraig12345 commented Mar 25, 2019 •

edited

Loading

ecraig12345 Mar 25, 2019

JasonGore Mar 26, 2019

ecraig12345 Mar 26, 2019

ecraig12345 Mar 26, 2019 •

edited

Loading

ecraig12345 Mar 26, 2019

ecraig12345 Mar 26, 2019

ecraig12345 Mar 26, 2019

JasonGore Mar 26, 2019

ecraig12345 Mar 26, 2019

ecraig12345 Mar 26, 2019

ecraig12345 Mar 26, 2019

size-auditor bot commented Mar 26, 2019 •

edited

Loading

KevinTCoughlin left a comment

JasonGore Mar 26, 2019

ecraig12345 Mar 26, 2019

JasonGore Mar 26, 2019

khmakoto left a comment

khmakoto commented Mar 26, 2019 •

edited

Loading

ecraig12345 commented Mar 26, 2019

khmakoto commented Mar 26, 2019

ecraig12345 commented Mar 27, 2019

msft-github-bot commented Mar 27, 2019

msft-github-bot commented Mar 27, 2019

msft-github-bot commented Mar 27, 2019

Fix styling issues found in example-app-base cleanup #8467

Fix styling issues found in example-app-base cleanup #8467

Conversation

ecraig12345 commented Mar 25, 2019 • edited Loading

Pull request checklist

Description of changes

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecraig12345 Mar 26, 2019 • edited Loading

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

size-auditor bot commented Mar 26, 2019 • edited Loading

KevinTCoughlin left a 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

khmakoto left a comment

Choose a reason for hiding this comment

khmakoto commented Mar 26, 2019 • edited Loading

ecraig12345 commented Mar 26, 2019

khmakoto commented Mar 26, 2019

ecraig12345 commented Mar 27, 2019

msft-github-bot commented Mar 27, 2019

msft-github-bot commented Mar 27, 2019

msft-github-bot commented Mar 27, 2019

ecraig12345 commented Mar 25, 2019 •

edited

Loading

ecraig12345 Mar 26, 2019 •

edited

Loading

size-auditor bot commented Mar 26, 2019 •

edited

Loading

khmakoto commented Mar 26, 2019 •

edited

Loading