Consistent scope and naming for variables, mixins, and keyframes #887

mikewheaton · 2017-01-03T23:10:21Z

Fixes #832.

This PR adds scoping (via the ms- prefix) and makes the naming of mixins, variables, and keyframes more consistent.

With the exception of the brand icon modifier classes (see below), there are no other changes to the CSS classes in Fabric, so while this is a breaking change it won't affect the majority of applications using Fabric as a simple CSS include.

New naming convention

Everything in the global scope is prefixed with "ms-".
CSS classes remain unchanged (following SUIT CSS conventions).
- ms-Grid-col
- Exception: Pascal case is used for the icon names: ms-Icon--EraseTool
Animation keyframes all match their corresponding CSS class.
- @keyframes ms-slideRightIn10
Variables are kebab case.
- $ms-animation-duration-3
- $ms-screen-min-xl
- Exception: camel case is used for colors to match class names: $ms-color-themeLighterAlt
Mixins are usually kebab case.
- ms-focus-border()
- ms-drop-shadow()
- Exception: If the mixin maps directly to a CSS class, we match the class name:
  - ms-u-borderBox() outputs to .ms-u-borderBox
  - ms-fontSize-s() outputs to .ms-fontSize-s

Breaking changes

Animation keyframes

Animation keyframes are now prefixed: slideRightIn10 to ms-slideRightIn10

Classes

Brand icon modifiers are now lowercase: ms-BrandIcon--Icon16 to ms-BrandIcon--icon16

Mixins

Prefixed all mixins: reset-padding to ms-reset-padding
Brand icons
- Renamed mixins to kebab case: brandIconClasses to ms-brand-icon-classes
Font
- Scoped all mixins: SegoeUIWestEuropeanLight to ms-font-family-segoeUIWestEuropeanLight
- Renamed language-override-system-fonts to ms-font-family-override
- Renamed ms-baseFont to ms-base-font
Directionality
- Renamed baseRtl to ms-base-RTL
General
- Renamed mixins to kebab case: resetAnimation to ms-reset-animation
Utility
- Renamed mixins to kebab case: resetMargins to ms-reset-margins

Variables

Responsive
- Changed order of screen size and min/max: ms-screen-sm-min to ms-screen-min-sm
Animation
- Added "animation" to scope: ms-ease2 to ms-animation-ease-2
Brand icon
- Switched to kebab case: ms-productImagesPath to ms-product-images-path
Font
- Renamed ms-font-family-base to ms-font-base-family
- Renamed ms-font-system-base to ms-font-base-system

This reverts commit 3b244b0.

…n/max

Jahnp · 2017-01-04T17:44:06Z

src/sass/mixins/_Directionality.Mixins.scss

      clear: left;
    }
  } @else {
      clear: $side;
  }
 }

-@mixin float($direction) {
+@mixin ms-float($direction) {


Hmm... this is going to cause a lot of upgrade pain. Are we confident this is the right change to make?

It's never fun when we introduce breaking changes, but it is a simple find-replace in this case. There's no change in functionality so once all of the build errors are caught it's good to go. I'm planning on doing the update in Fabric React and can help if other teams have concerns.

Jahnp · 2017-01-04T17:49:04Z

src/sass/mixins/_Directionality.Mixins.scss

    margin: $top $left $bottom $right;
  }
 }

-@mixin margin-left($distance) {
-  @include LTR {
+@mixin ms-marginLeft($distance) {


I realize the intent here is to be as consistent as possible across the library and to prevent any potential conflicts when using other SASS libraries, but the names of the directionality mixins were meant to match the CSS properties they affect to make their usage as transparent as possible. I won't block on this right now, but I again do worry that this will be a painful upgrade for a lot of users.

It's certainly a 'nice to have' that all of the mixins follow the same naming convention and not an absolute must. My thinking was that since we're breaking everything by adding the "ms-" prefix anyway, it's a good opportunity to standardize the name.

Another option could be to update our other mixins (the ones that don't directly output CSS classes, where we match the names) to use kebab-case.

After discussing in person, we're going to go ahead and kebab case all of the mixins while keeping the "ms-" prefix: ms-margin-left()

# Conflicts: # src/sass/_Font.scss # src/sass/mixins/_Animation.Mixins.scss

…ess they are strictly to output a CSS class)

…ventions

Jahnp · 2017-01-06T00:14:39Z

Approved

micahgodbolt · 2017-01-06T16:26:55Z

ghdocs/CONVENTIONS.md

+When naming classes, animation keyframes, variables, and mixins, please follow these conventions:
+
+- Everything in the global scope is prefixed with "ms-".
+- CSS classes remain unchanged (following [SUIT CSS conventions](https://github.com/suitcss/suit/blob/master/doc/naming-conventions.md)).


Line talks about things being unchanged. Good for PR notes, not needed for docs.

Mike Wheaton added 15 commits December 31, 2016 11:47

Rename icon web font and move name to variable

3b244b0

Scope all animation keyframes

ea4ddfa

Scope all mixins

86232df

Revert "Rename icon web font and move name to variable"

628bc15

This reverts commit 3b244b0.

Remove the utility prefix from all directionality mixins

f87aec6

Renamed all directionality mixins to camelcase without extra dashes

9c88b9e

Rename mixins to match naming convention (camelcase, all together)

7d5f535

Change BrandIcon modifiers to lowercase

a4b9886

Add “brandIcon” scope to all brand icon variables

6f84071

Rename font system and family base variables

151f3ef

Prefix all animation variables

fd7a388

Rename all responsive screen variables to change order of size and mi…

f4a485d

…n/max

Rename font system and font family base variables

0062d98

Rename brand icon mixins to match other mixins

15b7f8f

Update documentation with new mixin and variable names

c102640

msftclas added the cla-already-signed label Jan 3, 2017

Jahnp reviewed Jan 4, 2017

View reviewed changes

Mike Wheaton added 4 commits January 4, 2017 12:18

Merge branch 'master' into miwhea/consistent-names

31b7a2f

# Conflicts: # src/sass/_Font.scss # src/sass/mixins/_Animation.Mixins.scss

Merge branch 'master' into miwhea/consistent-names

7d43f02

Another pass at standardizing mixins and variables as kebab-case (unl…

4b23344

…ess they are strictly to output a CSS class)

Add documentation page for code conventions, starting with naming con…

12967c9

…ventions

mikewheaton requested review from gokunymbus, micahgodbolt and philworthington January 5, 2017 22:10

Mike Wheaton added 2 commits January 6, 2017 08:19

Small update to naming conventions document

2c4deee

Fix typo in naming conventions document

3ef8acc

micahgodbolt suggested changes Jan 6, 2017

View reviewed changes

Fix conventions documentation

bd4b408

micahgodbolt approved these changes Jan 6, 2017

View reviewed changes

mikewheaton added this to the 6.0.0 milestone Jan 6, 2017

mikewheaton merged commit 3dfc460 into master Jan 6, 2017

mikewheaton deleted the miwhea/consistent-names branch January 6, 2017 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent scope and naming for variables, mixins, and keyframes #887

Consistent scope and naming for variables, mixins, and keyframes #887

mikewheaton commented Jan 3, 2017 •

edited

Loading

Jahnp Jan 4, 2017

mikewheaton Jan 4, 2017

Jahnp Jan 4, 2017

mikewheaton Jan 4, 2017

mikewheaton Jan 4, 2017

mikewheaton Jan 4, 2017

Jahnp commented Jan 6, 2017 •

edited by dzearing

Loading

micahgodbolt Jan 6, 2017

Consistent scope and naming for variables, mixins, and keyframes #887

Consistent scope and naming for variables, mixins, and keyframes #887

Conversation

mikewheaton commented Jan 3, 2017 • edited Loading

New naming convention

Breaking changes

Animation keyframes

Classes

Mixins

Variables

Jahnp Jan 4, 2017

Choose a reason for hiding this comment

mikewheaton Jan 4, 2017

Choose a reason for hiding this comment

Jahnp Jan 4, 2017

Choose a reason for hiding this comment

mikewheaton Jan 4, 2017

Choose a reason for hiding this comment

mikewheaton Jan 4, 2017

Choose a reason for hiding this comment

mikewheaton Jan 4, 2017

Choose a reason for hiding this comment

Jahnp commented Jan 6, 2017 • edited by dzearing Loading

micahgodbolt Jan 6, 2017

Choose a reason for hiding this comment

mikewheaton commented Jan 3, 2017 •

edited

Loading

Jahnp commented Jan 6, 2017 •

edited by dzearing

Loading