bump office-ui-fabric-react to 5.85.0 by chrismohr · Pull Request #300 · microsoft/YamUI

chrismohr · 2018-04-21T01:19:56Z

In preparation to allow custom description's on TextField.
microsoft/fluentui#4588

Changes

Avatar - Persona initials changed line-height from 70px to 72px. Added overrided to fix.
https://github.com/OfficeDev/office-ui-fabric-react/pull/4382/files
MenuButton - Fabric IconButton refactor caused a change in css order. If you look at the commit history, my first fix was to use more specific selectors. Then instead I passed the overrides in through the styles prop on IconButton. I think this will generally be more maintainable, but don't feel great about the hard-coded color strings. Feedback?
Hovercard - .ms-Callout now has a 1px border, when we expected 0. I've added an !important overrride here.
Various snapshot changes.

https://github.com/OfficeDev/office-ui-fabric-react/pull/4382/files changed line-height from 70px to 72px

chrismohr · 2018-04-24T00:20:19Z

Ok, this is ready for review. @swese44 @unindented @richardtagger

unindented · 2018-04-24T01:30:58Z

src/components/MenuButton/MenuButton.tsx

+        opacity: 0.7,
+        width: `${this.props.iconSize}px`,
+        height: `${this.props.iconSize}px`,
+        color: '#495361',


It may be time to create a TS file with all these colors, and have it live side-by-side with the CSS one. Having these colors sprinkled in TS code doesn't look great.

i agree, i don't know what color that is just looking at the PR :(

unindented · 2018-04-24T01:32:20Z

src/components/MenuButton/MenuButton.tsx

+        opacity: 1,
+      },
+    };
+  };


You will be generating a new object every time this function gets called, which means you'll need to re-render this component every time.

Looks like getMenuProps had the same problem, so this isn't making it worse.

the styles can change if certain props change, so i guess we could cache the styles object and update it when props change. seems gross. another option could be to make the entire styles object static, specifying variable values like root width and height on specific root classes and passing the correct root className in at each size?

unindented · 2018-04-24T01:33:50Z

src/css/variables/borders.css

  box-shadow: 0 0 12px rgba(0, 0, 0, 0.2) !important;
  border: none;
  border-radius: 1px;
+  border-width: 0px !important;


No need for units when 0, just border-width: 0 !important.

swese44

i'd like to see us address colors in TS. if you want to ship it now then please add an issue so we can track it.

it would be nice to replace getStyles with a static object, but as @unindented pointed out we still have getMenuProps so it won't help with perf at this point

unindented · 2018-04-24T16:24:56Z

src/util/fabric/theme.ts

      lineHeight: '2.8rem',
    },
-    superLarge: fontStyles,
-    mega: fontStyles,


Weird thing is that it looks like these fontStyle colors had no effect.

these did override the default typography element colors. are visual diffs still passing?

Yes, the visual diffs still passed. I manually inspected some elements, but had trouble finding the rule being applied anywhere.

Looking closer now at Block, I see the color is being overridden by a merge style, so instead of #343A41 it is rgb(51, 51, 51).

I'll look a little closer at this, and see if our visual diff picks up the difference.

Also just to clarify, it's our current version of of yamui (https://microsoft.github.io/YamUI) that is using the wrong color.

I confirmed that #343A41 is not surfaced anywhere AND that our visual diff would detect the change. If we want #343A41 to be the default text color, it looks like we can add neutralPrimary: blackMetal, to our theme overrides.

swese44 · 2018-04-24T18:05:03Z

src/util/colors.ts

+  button: deathMetal,
+  buttonHovered: river,
+  buttonPressed: river,
+  buttonExpanded: river,


in CSS we abstracted all these colors into pseudo "theme" variables which components would map to their own rules. i think we're getting too specific here, i think we should be mapping our un-understandable blue/stream/pond/deathmetal color names to contextual theme names like Fabric does.

otherwise this colors object will turn into an enormous object with lots of duplication

@swese44 Makes sense. So maybe textColorSecondary and linkColor?

i think it would be ideal to just use the same color variables we had in CSS, it'll be easier for us to refactor CSS into mergeStyles in the future

(or just the subset of the existing color variables you need for button)

Cool. I got the names textColorSecondary and linkColor from the colors.css file, but just converted the name to camel case.

Is this what you had in mind?

const colors = { textColorSecondary: deathMetal, linkColor: river, };

the word color is probably a bit redundant. How about this?

const colors = { textSecondary: deathMetal, link: river, };

swese44

thanks!

bump office-ui-fabric-react to 5.85.0

2afe5f8

chrismohr requested review from adrianmg, mamuso, richardtagger, swese44 and unindented as code owners April 21, 2018 01:19

chrismohr added the wip label Apr 21, 2018

chrismohr changed the title ~~bump office-ui-fabric-react to 5.85.0~~ [wip] bump office-ui-fabric-react to 5.85.0 Apr 21, 2018

Chris Mohr added 4 commits April 23, 2018 11:32

update fabric to 5.26, and updaate snaps

d34e0f5

fix xLarge avatar initials vertical alignment

c8a2bcf

https://github.com/OfficeDev/office-ui-fabric-react/pull/4382/files changed line-height from 70px to 72px

increase specificity of MenuButton overrides

69f9663

js styles will likely be more robust than the css overrides

791c7b2

chrismohr force-pushed the jsx_for_description branch from 6f5843c to 791c7b2 Compare April 23, 2018 21:28

fix callout borders

6c35c6a

chrismohr force-pushed the jsx_for_description branch from c3e602c to 6c35c6a Compare April 23, 2018 23:43

chrismohr and others added 2 commits April 23, 2018 16:44

Merge branch 'master' into jsx_for_description

8bb1216

update snaps for style overrides

1843390

chrismohr changed the title ~~[wip] bump office-ui-fabric-react to 5.85.0~~ bump office-ui-fabric-react to 5.85.0 Apr 24, 2018

chrismohr removed the wip label Apr 24, 2018

unindented reviewed Apr 24, 2018

View reviewed changes

swese44 approved these changes Apr 24, 2018

View reviewed changes

define colors in one ts file

0e451df

chrismohr force-pushed the jsx_for_description branch from 965388e to 0e451df Compare April 24, 2018 16:23

unindented reviewed Apr 24, 2018

View reviewed changes

chrismohr force-pushed the jsx_for_description branch from 9f0697b to 0e451df Compare April 24, 2018 17:57

Merge branch 'master' into jsx_for_description

fa0cd25

swese44 reviewed Apr 24, 2018

View reviewed changes

Chris Mohr added 2 commits April 24, 2018 11:56

use color names that align with our existing colors.css file

2987e6f

px unecessary for 0

609d577

swese44 approved these changes Apr 24, 2018

View reviewed changes

fix dropdown item alignment

14eda72

chrismohr merged commit 2137fa7 into master Apr 24, 2018

chrismohr deleted the jsx_for_description branch April 24, 2018 21:34

chrismohr mentioned this pull request Apr 25, 2018

Release version 6.2.0 #304

Merged

Conversation

chrismohr commented Apr 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

chrismohr commented Apr 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swese44 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrismohr Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swese44 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrismohr commented Apr 21, 2018 •

edited

Loading

chrismohr Apr 24, 2018 •

edited

Loading