Styling: integrating glamor based styling, removing fabric-core dependency #1837

dzearing · 2017-05-23T00:14:17Z

Addresses #983

Description of changes

Problems we're aiming to solve:

Ensure all components work out of the box. No dependency on global css.
All css rules are versioned.
Open up a path for richer styling which has better control over rule specificity and RTL sensitivity.
Support a more robust theming approach.

This change does a couple of things:

It creates a dependency on the @uifabric/styling package. This package is still shuffling, but we want to take an early dependency on it and gradually migrate our CSS story.
It completely removes all global css references. There are zero references to ms-font-, ms-u-animation-, ms-icon-*. That means out of the box things will work correctly without needed stylesheets.
Redoes the button styles using glamor. The existing rules have remained intact; we just use glamor to register them. And, if you provide custom rules, we have an opportunity to mix those in, and ensure that we control specificity; the overrides always win.

…c-react into styling-updates

… This will help identify how we're impacting the bundle size of the button, and will help validate that we're actually able to render a button without fabric-core.

… replacement css utility that works better mixing glamor styling and class name strings together.

…erstandable rules that don't rely on non-contractual css child selectors to do anything. Updated Primary button to not use sass.

…fabric-core.

…e Icon component!

…c-react into styling-updates # Conflicts: # apps/fabric-website/package.json # common/npm-shrinkwrap.json # common/temp_modules/rush-example-app-base/package.json # common/temp_modules/rush-example-component/package.json # common/temp_modules/rush-fabric-website/package.json # common/temp_modules/rush-office-ui-fabric-react/package.json # common/temp_modules/rush-styling/package.json # common/temp_modules/rush-todo-app/package.json # packages/example-app-base/package.json # packages/office-ui-fabric-react/package.json # packages/office-ui-fabric-react/src/components/pickers/BasePicker.tsx # packages/office-ui-fabric-react/src/utilities/decorators/withContainsFocus.tsx # packages/styling/package.json # tslint.json

…c-react into styling-updates # Conflicts: # apps/fabric-website/package.json # common/npm-shrinkwrap.json # common/temp_modules/rush-example-app-base/package.json # common/temp_modules/rush-fabric-website/package.json # common/temp_modules/rush-office-ui-fabric-react/package.json # common/temp_modules/rush-styling/package.json # packages/example-app-base/package.json

…c-react into styling-updates # Conflicts: # common/npm-shrinkwrap.json # packages/example-app-base/package.json # packages/office-ui-fabric-react/package.json

…c-react into styling-updates # Conflicts: # packages/office-ui-fabric-react/src/components/Persona/examples/Persona.Basic.Example.tsx

christiango

I'm always a fan of a change that removes more code than it adds

christiango · 2017-05-23T15:16:05Z

packages/office-ui-fabric-react/src/components/Breadcrumb/Breadcrumb.tsx

@@ -148,7 +157,7 @@ export class Breadcrumb extends BaseComponent<IBreadcrumbProps, IBreadcrumbState
  }

  @autobind
-  private _onOverflowClicked(ev: React.MouseEvent<HTMLElement>) {


Reason for dropping the type here?

christiango · 2017-05-23T15:19:32Z

packages/office-ui-fabric-react/src/components/Button/Button.Props.ts

@@ -12,6 +13,32 @@ export interface IButton {
  focus: () => void;
 }

+export interface IButtonStyles {


Some comments to help people realize which is the appropriate style to override for customization?

christiango · 2017-05-23T15:24:11Z

packages/office-ui-fabric-react/src/components/Button/IconButton/IconButton.styles.ts

+const DEFAULT_BUTTON_HEIGHT = '32px';
+const DEFAULT_PADDING = '0 4px';
+
+export function getStyles(


No memoize?

christiango · 2017-05-23T15:28:14Z

packages/utilities/src/rtl.ts

@@ -40,15 +40,15 @@ export function getRTL(): boolean {
 /**
 * Sets the rtl state of the page (by adjusting the dir attribute of the html element.)
 */
-export function setRTL(isRTL: boolean) {
+export function setRTL(isRTL: boolean, avoidPersisting = false) {


micahgodbolt · 2017-05-23T16:43:54Z

pulling this down for a look as well

micahgodbolt · 2017-05-23T18:57:02Z

packages/office-ui-fabric-react/src/components/Button/PrimaryButton/PrimaryButton.styles.ts

+  getStyles as getDefaultButtonStyles
+} from '../DefaultButton/DefaultButton.styles';
+
+export function getStyles(


This feels like a bunch of boilerplate. theme, custom styles, palette...can we/should we abstract some of that away?

export function getStyles( theme: ITheme = getTheme(), customStyles?: IButtonStyles, ): IButtonStyles { let { palette } = theme;

I'm not really sure. Styles are a function of the theme and of customStyles.

It is possible that we can pull the "global" theme, but it removes the ability to override the theme in a context.

It is possible to not take in customStyles, and to instead mix them in from within the component itself.

not saying to change any of the functionality. custom themes are obviously important. Just curious if we can wrap it up and abstract what I'm sure will end up in just about every styles file.

If you think this is about as abstract as you want to get right now, no worries. We can always tweak it later.

micahgodbolt

Other than a small question about function abstraction, i love this. It could also use an example where we override some styles.

…c-react into styling-updates # Conflicts: # common/temp_modules/rush-example-app-base/package.json # common/temp_modules/rush-office-ui-fabric-react/package.json # common/temp_modules/rush-utilities/package.json # packages/example-app-base/package.json # packages/office-ui-fabric-react/package.json # packages/utilities/package.json

…fficedev/office-ui-fabric-react into styling-updates # Conflicts: # packages/example-app-base/package.json

…c-react into styling-updates

… like autobind. Updating things accordingly.

…xed mergeStyleSets so that you don’t have to specify “undefined” for non styled things in the base class.

* add semantic colors and convert nonhover toggle colors to use them * fix lint error * address code review feedback and better comment * wip: finishing up toggle, still have disabled thumb color bug to address * update toggle to new way of doing things * code review feedback * cleanup * umm, updates and stuff * cleanup for merge * fix lint errors

…c-react into styling-updates # Conflicts: # packages/example-app-base/package.json # packages/office-ui-fabric-react/package.json

…b of Toggle things.

…c-react into styling-updates

…c-react into styling-updates # Conflicts: # common/temp_modules/rush-example-app-base/package.json # common/temp_modules/rush-office-ui-fabric-react/package.json # common/temp_modules/rush-styling/package.json # common/temp_modules/rush-utilities/package.json # packages/example-app-base/package.json # packages/office-ui-fabric-react/package.json # packages/office-ui-fabric-react/src/components/Button/BaseButton.scss # packages/office-ui-fabric-react/src/components/Callout/CalloutContent.tsx # packages/office-ui-fabric-react/src/components/Check/Check.scss # packages/office-ui-fabric-react/src/components/DetailsList/DetailsRow.tsx # packages/office-ui-fabric-react/src/components/Fabric/Fabric.tsx # packages/office-ui-fabric-react/src/components/Image/Image.tsx # packages/office-ui-fabric-react/src/components/Layer/examples/Layer.Basic.Example.tsx # packages/office-ui-fabric-react/src/components/Layer/examples/Layer.Hosted.Example.tsx # packages/office-ui-fabric-react/src/components/List/examples/List.Mail.Example.scss # packages/office-ui-fabric-react/src/components/Nav/Nav.tsx # packages/office-ui-fabric-react/src/components/Panel/Panel.tsx # packages/office-ui-fabric-react/src/components/TextField/TextField.tsx # packages/office-ui-fabric-react/src/components/Tooltip/Tooltip.tsx # packages/styling/package.json

…-ui-fabric-react into styling-updates # Conflicts: # packages/example-app-base/package.json # packages/office-ui-fabric-react/package.json # packages/office-ui-fabric-react/src/components/Toggle/Toggle.scss

dzearing added 30 commits April 27, 2017 13:40

Updating rush generate to include styling.

1b1c04a

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

ab5a1fc

…c-react into styling-updates

Adding a test-bundle-button package to test bundling only the button.…

8fdfbad

… This will help identify how we're impacting the bundle size of the button, and will help validate that we're actually able to render a button without fabric-core.

Updating typescript build errors due to new shrinkwrap; also adding a…

f88bd36

… replacement css utility that works better mixing glamor styling and class name strings together.

Updating the classname contract for buttons to have more specific und…

a1d907c

…erstandable rules that don't rely on non-contractual css child selectors to do anything. Updated Primary button to not use sass.

Updating all buttons to use styling.

e5197e9

Removing unnecessary files. Cleaning up lint.

ca95172

Updates to linting.

130ae9c

More cleanup; adding mergeRules helper to reduce mistakes .

e6cd63e

Removing redundant button definitions.

89a3f9d

More changes, better type safety.

f8da743

Completely removing the fabric-core dependency for icons.

627b33d

Updating sourcemap loader.

2dda3f1

Fixing Panel animations completely. Fixing Callouts to not depend on …

620f300

…fabric-core.

Replacing random ms-Icon usage with styling icons.

77c6ed6

Fixing icon usage in chevron.

965564d

Removing more references to ms-icon.

a0b4c06

Removing icons and animations from nav.

ea5f9f9

Fixing the example header css.

2effbbf

Removing more font class name usage.

bea4e47

Fixing a number of cases that rely on ms-Icon--* classnames to use th…

84f33c3

…e Icon component!

Fixing lint.

e6a0f70

Fixing the panel test.

0923b90

Fixing lint.

cc521bc

Updates to all of the things.

2004ebf

Fixing lint.

36df863

Updates to typings and usage.

f8f5b80

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

3ed1627

…c-react into styling-updates # Conflicts: # common/npm-shrinkwrap.json # packages/example-app-base/package.json # packages/office-ui-fabric-react/package.json

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

8c91541

…c-react into styling-updates # Conflicts: # packages/office-ui-fabric-react/src/components/Persona/examples/Persona.Basic.Example.tsx

msftclas added the cla-not-required label May 23, 2017

christiango approved these changes May 23, 2017

View reviewed changes

micahgodbolt reviewed May 23, 2017

View reviewed changes

micahgodbolt approved these changes May 23, 2017

View reviewed changes

dzearing and others added 20 commits May 23, 2017 16:24

Variety of fixes for memoize and button style mixing.

ad7c6fe

Merge branches 'styling-updates' and 'master' of https://github.com/o…

bc8e0a1

…fficedev/office-ui-fabric-react into styling-updates # Conflicts: # packages/example-app-base/package.json

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

a60f974

…c-react into styling-updates

Fixing memoize to work with arbirtrary arguments.

3171c92

Renaming memoize to memoizeFunction, and making “memoize” a decorator…

627e831

… like autobind. Updating things accordingly.

Updating IStyle to include arrays of raw or processed styles.

fa2f46c

Fixing typings.

da93410

Flattened hovered/active styling to be part of the style contract. Fi…

7299252

…xed mergeStyleSets so that you don’t have to specify “undefined” for non styled things in the base class.

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

c1c200f

…c-react into styling-updates # Conflicts: # packages/example-app-base/package.json # packages/office-ui-fabric-react/package.json

Updating memoize to set “this” to null to prevent bugs, complete scru…

3dac477

…b of Toggle things.

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

5ad9552

…c-react into styling-updates

Adding change file and such.

e4708b8

Deprecating button toggled in favor of checked.

b66e333

missed save.

412d161

updating shrinkwrap

fb93e47

Updating how font families are loaded based on the language of the page.

6ad626e

dzearing merged commit 3a198d4 into master May 29, 2017

dzearing deleted the styling-updates branch May 29, 2017 19:29

whykushal93 mentioned this pull request Jul 13, 2017

Moving Checkbox component to use the Glamor-based styling package. #2202

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Styling: integrating glamor based styling, removing fabric-core dependency #1837

Styling: integrating glamor based styling, removing fabric-core dependency #1837

dzearing commented May 23, 2017 •

edited

Loading

christiango left a comment

christiango May 23, 2017

christiango May 23, 2017

christiango May 23, 2017

christiango May 23, 2017

micahgodbolt commented May 23, 2017

micahgodbolt May 23, 2017 •

edited

Loading

dzearing May 23, 2017

micahgodbolt May 23, 2017

micahgodbolt left a comment

Styling: integrating glamor based styling, removing fabric-core dependency #1837

Styling: integrating glamor based styling, removing fabric-core dependency #1837

Conversation

dzearing commented May 23, 2017 • edited Loading

Description of changes

christiango left a comment

Choose a reason for hiding this comment

christiango May 23, 2017

Choose a reason for hiding this comment

christiango May 23, 2017

Choose a reason for hiding this comment

christiango May 23, 2017

Choose a reason for hiding this comment

christiango May 23, 2017

Choose a reason for hiding this comment

micahgodbolt commented May 23, 2017

micahgodbolt May 23, 2017 • edited Loading

Choose a reason for hiding this comment

dzearing May 23, 2017

Choose a reason for hiding this comment

micahgodbolt May 23, 2017

Choose a reason for hiding this comment

micahgodbolt left a comment

Choose a reason for hiding this comment

dzearing commented May 23, 2017 •

edited

Loading

micahgodbolt May 23, 2017 •

edited

Loading