Restructure, tidy, and bug fix SCSS

[mikewheaton]

This PR restructures the sass folder to remove the confusing "output" file names (fixes #349), giving us a new structure like so:

- sass
  - mixins
    - _Animation.Mixins.scss
    ...
  - variables 
    - _Animation.Variables.scss
    ...
  _Animation.scss
  Fabric.scss
  ...

All of the variables and mixins are clearly separated out from the partials that actually generate CSS. Those partials use the underscore naming convention (e.g. _Grid.scss) to indicate that they shouldn't be converted directly to CSS. The only files without the underscore are Fabric.scss and Fabric.RTL.scss.

Along the way, I discovered several other bugs and inconsistencies, many of which are fixed by this PR. Here are the changes and fixes:

No impact on output CSS or use of mixins/partials

• Standardized to use single quotes everywhere.
• Removed _Fabric.i18n.scss. We replaced this with another file a while back, now located in mixins/_Directionality.Mixins.scss, but this original hadn't been deleted. Fortunately, it was loaded before the proper mixins and had no effect on the output.
• Added missing semicolons, fixed indentation, etc.
• Renamed the animation slideLeft400 to slideLeftIn400 to match the others. This is a change only in the @keyframes name and does not impact the mixins or class names.
• Fabric.RTL.scss now simply imports Fabric.scss and then references RTL-specific overrides. Reduces the chance of future bugs caused by adding a reference to one file and not the other.

Changes to mixins/partials

• Many mixins/partials have been renamed, moved, merged together, etc. This will be a breaking change for anyone importing our SCSS files into their project. However, the improved structure and naming should make it easy to update references.
• Removed some mixins that created browser prefixes for animations. This is handled by Autoprefixer so the mixins are not necessary.
• Added a new mixin, ms-animation-keyframes(), that outputs all of the @keyframes used by the animation classes. Previously, you could use mixins to add animation to an element, but that animation wouldn't work because the @keyframes were unavailable.
• Renamed the animationMix() mixin to ms-animation(). I'll be renaming more mixins and variables in a future PR to fix Scope all mixins and variables, and name them consistently #832, as we're inconsistent here and should be scoping them all.
• Renamed _Fabric.Base.scss to _Wrapper.scss, so we can consistently refer to it as the 'wrapper' class. No change to the actual output of ms-Fabric.
• Renamed _Fabric.Common.scss to _References.scss and made sure this contains all of the variables and mixins. Then imported this file into Fabric.scss so that no references are missed.
• The ms-Grid-col() mixin now styles child .ms-Grid classes, so that grids can be nested correctly.
• Renamed the Typography files to Font to match the actual variable/mixin/class names.

Differences in the output CSS

• Animation classes no longer contain browser prefixes. These are not needed for the browsers we support. If prefixes are needed in the future, this will be taken care of by Autoprefixer.
• fabric.css now contains the language overrides for font stacks (fixes Language font stack overrides missing from fabric.css #865).
• fabric.rtl.css contained many animation classes that were referring to @keyframes by the wrong name. This meant that the animations weren't working in RTL. They now refer to the correct animations: slideLeftIn10, slideLeftIn20, slideLeftIn40, slideRightIn10, slideRightIn20, slideRightIn40, and slideRight400.
• fabric.rtl.css now includes the BrandIcon classes (fixes Brand Icons missing from fabric.rtl.css #866).

With so many file changes it's hard to look at the diffs, so I've used the super-helpful Alan's Compare CSS tool to compare the master versions of fabric.css and fabric.rtl.css with the versions from this branch. I went through several times to be sure I'm not introducing any unintended changes.

[micahgodbolt]

I still believe file names should include their category (animation-variable,animation-mixins, animation-classes etc). It'll help a ton in fuzy searching and code review.

[mikewheaton]

@micahgodbolt: So your preference on file names would be _Animation.Variables.scss, _Animation.Mixins.scss, and _Animation.scss? I can get behind that. 😄 Beats having identical file names and having to rely on the folder to know which is which.

[Jahnp]

Agreed, it's useful to distinguish filenames when doing fuzzy search in editors:

Or in open tabs:

[Jahnp]

Thanks for doing this @mikewheaton! I'll pull down your branch to check out your changes soon. So excited to finally get these cleaned up! Things were starting to get huge and crusty.

[mikewheaton]

@micahgodbolt @Jahnp: I'd also like feedback on _References.scss, which used to be _Fabric.Common.scss. Is the name an improvement? My concern was that "common" suggested these were popular parts of Fabric, when it's just imports for all of the variables and mixins.

Would it make sense to see something like @import 'office-ui-fabric-core/References'; at the top of an SCSS file?

[Jahnp]

General feedback: is there a reason the root-level "output" files are prefixed with an underscore like partials? I understand that they are still generating CSS, but unless I'm missing something, it seems odd for them to have that prefix.

[Jahnp]

Totally understand the motivation here and agree that the original name/value mismatch for these was confusing, but I'm quite sure at the time the values were intentional/by design. While it's probably fine to change them to make more sense, please check with motion design to verify.

Frankly, I'd prefer we use simple, predictable durations in sensible increments like you've proposed here.

[mikewheaton]

@Jahnp: The underscore in Sass/SCSS indicates a partial, which is any file that is included by others and doesn't directly output a CSS file. Our build system doesn't care about the underscore, so it really makes no difference, but I wanted to follow this convention that others might recognize. It's also used by Bootstrap and Foundation.

[Jahnp]

@mikewheaton: I see--so it prevents there from being a Grid.css file being generated, but doesn't preclude a _Grid.css file from still producing output. Makes sense, I had forgotten that distinction.

[mikewheaton]

I've updated the variable and mixin file names to include their type (e.g. _Responsive.Variables.scss).

[Jahnp]

@mikewheaton I also hear you on the reasoning behind the naming of _References.scss. My only "concern" would be that "Common" seems to be a, well, common name for files that import mixins and variables used across a project. I see this internally and externally on a number of projects, but if I'm honest, I don't think it's a big deal.

I think the biggest thing would be to just make sure we call out the breaking change, because I know there are many projects that would be affected by this.

[mikewheaton]

@Jahnp: I'm not strongly against "Common", I just preferred "References" a bit more. It will be a breaking change in any case because the old files were all prefixed with "Fabric", so it's a good opportunity to change the name if we want to.

[Jahnp]

Approved

Thanks again for doing this @mikewheaton!

[micahgodbolt]

@mikewheaton sweet work here! Looking forward to seeing this repo being soooo much better organized.

[notandrew]

Should that be 'Segoe UI West European'?

[mikewheaton]

This will be investigated more closely in #382. Do you remember if 'Segoe UI WestEuropean' is supposed to refer to a locally-installed font? We have a web font for it on the CDN, but I can't find a corresponding @font-face declaration that makes this available.

[notandrew]

No, 'Segoe UI' is the local system font ($ms-font-system-base fonts are all local system fonts). 'Segoe UI West European' is the friendly name for the web font. Not sure why it's 'WestEuropean' here (unless I missed something in the last several months.)

[mikewheaton]

The web font is spelled as 'WestEuropean': https://github.com/OfficeDev/office-ui-fabric-core/blob/master/src/sass/mixins/_Font.Mixins.scss#L141

I'm going to open a new issue to include "Web Font" in the name of any @font-face we declare. It's difficult to tell right now where we're expecting a system font to load and where it's a web font we've defined, which we can name whatever we like. 😄

[notandrew]

oops. nevermind.

[notandrew]

Is this intentionally tabbed out like 8 spaces per tab? Or is it the github viewer that is making look gigantic?

[mikewheaton]

Good catch. Looks like this file is using tabs instead of spaces. We're going to be adding linting (#869) soon to keep inconsistencies like this from getting checked in.

[elisechant]

to me this namespacing looks like a vendor prefix and makes it more vague

[mikewheaton]

I agree that the "ms" prefix isn't the best indicator of "this belongs to Fabric". There are some legacy reasons for this and we'd likely choose another prefix if we were starting fresh today. However, at this point we use that prefix on all of our CSS classes and many variables and mixins, so changing it would be a major hassle for developers updating to new versions. Our goal, for now, is to at least be consistent, even if the prefix is slightly confusing.