-
Notifications
You must be signed in to change notification settings - Fork 467
Add 'ms-Fabric' base/wrapper component #784
Add 'ms-Fabric' base/wrapper component #784
Conversation
@@ -5,7 +5,7 @@ | |||
// -------------------------------------------------- | |||
// Fabric Core Typography mixins | |||
|
|||
@mixin ms-baseFont() { | |||
@mixin ms-baseFont { | |||
font-family: $ms-font-family-base; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to set this just yet. What was the reason for including it here now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just seemed reasonable to set the base font-family along with color. We can leave it out and discuss more if you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agreed. The only reason I'm questioning it now is that some teams consuming Fabric right now use separate definitions/font family names for Segoe than what Fabric currently provides, so this would produce work for them to override. Happy to discuss though--this definitely moves us in the right direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take it out for now and we can discuss more. I definitely want to set the base font family here, as it will greatly simplify how we do localization and even (finally!) allow for localized fonts in the Fabric JS components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mikewheaton. Like I said, I think it's absolutely the right thing to add, and you're absolutely right--this will accrue even greater benefits to other aspects of the project. My concern is that since this will be potentially broadly-impacting, I want to be sure we're careful about which properties we add when and that the appropriate consumers are alerted to the change and can plan accordingly.
This PR adds an
ms-Fabric
base/wrapper component that sets the base font-family and color.Once PR #766 is merged and the base font classes (e.g.
ms-font-m
) no longer have color baked in, this will be the preferred way of setting a base color for the entire page.It also gives us a single selector to target for localization, standardized focus styles, normalize/reset styles, and any other app-wide styles we want to apply in the future. Lots of possibilities here.