Convert Overlay to mergeStyles by jordandrako · Pull Request #3978 · microsoft/fluentui

jordandrako · 2018-02-14T22:22:01Z

Pull request checklist

Include a change request file using $ npm run change

Description of changes

Converts Overlay and it's examples to mergeStyles. Add theme support.
Add whiteTranslucent40 to theme palette.

Focus areas to test

Test if Overlay and any component that uses it as a subcomponent still function properly.

dzearing · 2018-02-15T03:03:29Z

+      className
+    ],
+
+    // Insert className styles


I think this comment was meant for above 49?

dzearing · 2018-02-15T03:03:58Z

 }

-export interface IOverlayProps extends React.HTMLAttributes<HTMLElement> {
+export interface IOverlayProps extends React.Props<OverlayBase> {


Was there a reason for this change?

I was experimenting with typings and forgot to change it back, though, this is the pattern other components use. Thanks for catching this.

oengusmacinog-zz · 2018-02-15T18:11:23Z

+  IOverlayStyles,
+} from './Overlay.types';
+
+import { classNamesFunction } from '../../Utilities';


this import can just move up with the others

oengusmacinog-zz · 2018-02-15T18:29:35Z

+
+        selectors: {
+          [HighContrastSelector]: {
+            border: '1px solid WindowText',


Is WindowText a valid choice here? This may be High Contrast Mode specific, but I just imagine it would be better to use a theme color.

Yup, it's HighContrast specific.

oengusmacinog-zz · 2018-02-15T18:36:42Z

@@ -1,41 +1,13 @@
-import * as React from 'react';
+import { styled } from '@uifabric/utilities';


I think we wanted to do import { styled } from '../../Utilities';

Nice catch.

oengusmacinog-zz · 2018-02-15T18:50:20Z

+        }
+      },
+
+      isNone && {


Process question: From what I can tell this was never actually applied before (just an unused SCSS class), and you went ahead and applied that as an optional prop. Do we want to do this, or assume abandoned functionality was abandoned for a reason. Like in this case does it serve a purpose or is it just a superfluous prop taking up bit space.

Good question. @battletoilet

phkuo · 2018-02-16T22:10:25Z

+  /**
+   * Style for the root element.
+   */
+  root?: IStyle;


don't we want all classNames to be required? @dzearing @mikewheaton

that way we can insure, inside getStyles, that we're making the entire thing. and then other consumers customizing it can just say Partial if they're merging with the existing one

In this case, having them required prevents customizing the styles via the customStyles prop.

Could you point me to an example of Partial styles?

I would like for us to have a way to insure any and all classNames defined in IFooStyles gets defined in the default implementation, while allowing other implementations to use a Partial<> to only have to override a subset.
I don't have any examples (we barely do any style customization right now as it is). Where is the customStyles prop used, in getStyles? Does this work?
getStyles: IStylesFunction<IFooProps, Partial>

Excuse me, the Component I was thinking of that uses customStyles is Checkbox, which I have another PR out for. I'll keep Partial in mind.

mikewheaton · 2018-02-16T22:14:30Z

+  const {
+    className,
+    theme,
+    isNone,


Could this be renamed for clarity? Maybe reverse it and call it isVisible?

Agreed, it's not used at the moment so that should be remedied.

Can we remove it entirely then?

I had the same question but forgot to follow up on it. @battletoilet - thoughts?

mikewheaton · 2018-02-16T22:15:50Z

+        visibility: 'hidden',
+      },
+
+      isDarkThemed && [


Could we rename this to isDark? That would match the CSS class below, and remove any potential confusion around this prop and theming.

I'll have to deprecate it but sure.

Even if we don't change the component's prop, this style prop could be changed for now right?

mikewheaton

Approved with minor comments.

* master: (71 commits) Applying package updates. Delete initials_2018-02-07-13-49.json Delete initials_2018-02-07-13-49.json Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json Delete initials_2018-02-07-13-49.json Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json Delete initials_2018-02-07-13-49.json Update .npmrc Cleaning Up Console Log in CommandBar Test (microsoft#4011) Convert Overlay to mergeStyles (microsoft#3978) ScrollablePane SCSS to MergeStyles Part 1: File Structure (microsoft#4008) [ContextualMenu] Fixes useTargetWidth property (microsoft#3943) DetailsList: Consider groups when setting aria-rowcount List: Add a _notifyPageChanges function (microsoft#3990) Applying package updates. Migrating Coachmark to main Package, Added a beak component and updated experiment PositioningContainer. (microsoft#3919) Update package.json Added enum for triggering menu with arrow keys and bool to allow it or not (microsoft#3950) BaseExtendedPicker: Hook up onPaste (microsoft#3885) FocusUtil: fix getPreviousElement to include previous sibling elements correctly (microsoft#3928) ...

Jordan Janzen added 13 commits February 12, 2018 16:25

Create base

1a437dd

Add whiteTranslucent40 to the palette.

5e96f55

Convert overlay sass to styles.ts

c194e12

Change Overlay to return styled OverlayBase

b9ed951

User getClassnames instead of styles. Move logic to styles.ts

8bab64c

Add style props

bbd14ae

Add customizable decorator. Remove css logic.

f33d62d

White instead of black default.

0d95ef2

Export base component

80f313c

Convert examples to mergeStyles too

d3936dd

Delete sass files

44bd6bd

npm run change output

5472fa3

Merge branch 'master' into mergeStyles/Overlay

900b7e7

jordandrako requested a review from phkuo as a code owner February 14, 2018 22:22

jordandrako requested review from mikewheaton and oengusmacinog-zz February 14, 2018 22:22

dzearing reviewed Feb 15, 2018

View reviewed changes

dzearing approved these changes Feb 15, 2018

View reviewed changes

Remove comment

9318d3f

oengusmacinog-zz suggested changes Feb 15, 2018

View reviewed changes

Jordan Janzen added 2 commits February 15, 2018 11:11

Fix some imports

fe611c9

Revert extended type change.

0b38726

oengusmacinog-zz approved these changes Feb 16, 2018

View reviewed changes

phkuo reviewed Feb 16, 2018

View reviewed changes

phkuo approved these changes Feb 16, 2018

View reviewed changes

mikewheaton reviewed Feb 16, 2018

View reviewed changes

mikewheaton approved these changes Feb 16, 2018

View reviewed changes

Rename isDarkThemed style prop to isDark.

24bea97

Make Root className required.

9733340

jordandrako merged commit 1f1db9d into microsoft:master Feb 17, 2018

jordandrako deleted the mergeStyles/Overlay branch February 17, 2018 00:11

jordandrako restored the mergeStyles/Overlay branch February 21, 2018 22:35

micahgodbolt added this to the Convert all components to css-in-js milestone Jul 2, 2018

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

		@@ -1,41 +1,13 @@
		import * as React from 'react';
		import { styled } from '@uifabric/utilities';

Conversation

jordandrako commented Feb 14, 2018

Pull request checklist

Description of changes

Focus areas to test

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!

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!

mikewheaton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants