-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Adding a dedicated merge-styles package which will replace the glamor usage. #2527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
packages/merge-styles/tsconfig.json
Outdated
"experimentalDecorators": true, | ||
"importHelpers": true, | ||
"noImplicitAny": true, | ||
"strictNullChecks": true, |
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.
probably worth just setting strict: true since it is a new project
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.
+1
export function concatStyleSets<T extends {}>(...args: (T | false | null | undefined)[]): T { | ||
let mergedSet: IStyleSet = {}; | ||
|
||
for (let i = 0; i < args.length; i++) { |
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.
args.foreach()?
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.
for (const currentSet of args) {
?
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.
+1 for for (const ___ of ___)
let rootStyle: IStyle = rules && rules.get('&'); | ||
|
||
// tslint:disable-next-line:no-any | ||
return rootStyle && (rootStyle as any).displayName; |
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.
why any
? can't we add displayName
to the type?
value | ||
]; | ||
|
||
let ruleEntries: string[] = []; |
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.
nit, move this down to where it's used?
} | ||
|
||
// Kebab case the rule. | ||
let rules: string[] = [ |
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.
Shouldn't we use an actual type instead of the += 2
logic everywhere we touch rulePairs
?
(if not, rename rules
to rulePairs
here?)
rules: Map<string, {}> = new Map<string, {}>(), | ||
currentSelector: string = '&' | ||
): Map<string, {}> { | ||
let stylesheet = Stylesheet.getInstance(); |
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.
const
|
||
// If the arg is a string, we need to look up the class map and merge. | ||
if (typeof arg === 'string') { | ||
let expandedRules = stylesheet.argsFromClassName(arg); |
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.
const
? (Maybe enable theconst
tslint rule?)
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 don't really agree with it. Here's why.
const
is very useful for communicating and enforcing that something should not change. There are times where you do this to avoid mistakes. I look at it as a readonly property. Good usage examples are things such as constant strings at the top of a file, which are reused in a variety of places. It communicates and enforces that these aren't going to change values ever.
But most of the time, you don't care to communicate this. And it doesn't prevent bugs when enforced. It just creates noise. I don't think I've ever hit a scenario where I accidentally assigned a value to something where it wasn't anticipated.
If you have a scenario where there is a legitimate bug caught by overusing const, I'd love to hear it.
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.
Maybe it's only because using C++ for years (const refs!), but for me it's valuable additional information, I don't actually have to read the rest of the code when I see const
, I know that variable is not being reassigned. Granted, if it's a reference to an object it's less useful since the object itself might change, but for "value types" it's valuable.
Don't remember any complicated bug being caught by it, it's more like surfacing issues in badly written code, inline anonymous functions that modify variables in the closure by accident and things like that.
for (let propName in ruleEntries) { | ||
if (ruleEntries.hasOwnProperty(propName)) { | ||
hasProps = true; | ||
serialized.push(propName); |
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.
serialized.push(propName, ruleEntries[propName]);
export function mergeStyles(...args: (IStyle | IStyle[])[]): string; | ||
|
||
// @public | ||
export function mergeStyleSets < T extends {} >(...cssSets: T[]): {[P in keyof T]?: string }; |
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 think you can use object
instead of {}
where you expect actual objects now.
|
||
case InjectionMode.appendChild: | ||
FabricPerformance.measure('appendChild', () => { | ||
element.appendChild(document.createTextNode( |
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.
Are we going to want a way to point this at a different document
instance, such as an iframe.contentWindow.document
?
// tslint:disable-next-line:no-any | ||
private _classNameToArgs: { [key: string]: any }; | ||
|
||
public static getInstance(): Stylesheet { |
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.
Does GlobalSettings
support targeting a different window
or document
instance?
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.
removed GlobalSettings
usage. If window is unavailable i cache it in the local module.
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.
by simplifying some of the logic i'm able to bring the total size down from 3.4k to 2.7k (gzipped) so that's good.
* @public | ||
*/ | ||
export function concatStyleSets<T extends {}>(...args: (T | false | null | undefined)[]): T { | ||
let mergedSet: IStyleSet = {}; |
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.
object
} | ||
|
||
function getDisplayName(rules?: Map<string, IStyle>): string | undefined { | ||
let rootStyle: IStyle = rules && rules.get('&'); |
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.
const rootStyle: IStyle & {
displayName?: string;
} = ...;
rules: Map<string, {}> = new Map<string, {}>(), | ||
currentSelector: string = '&' | ||
): Map<string, {}> { | ||
let stylesheet = Stylesheet.getInstance(); |
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 would prefer it if most of these functions took a Stylesheet
as a parameter rather than referring back to Stylesheet.getInstance()
. It would produce a more decoupled architecture that would be easier to extend.
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 on that one. Stylesheet tracks state over time, so by default it should be a singleton, but is there a use case where it would be better to pass around?
currentRules = {}; | ||
rules.set(currentSelector, currentRules); | ||
} | ||
|
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.
for (const arg of args) {
...
let serialized: string[] = []; | ||
let hasProps = false; | ||
|
||
rules.forEach((ruleEntries: { [key: string]: string }, selector: string): void => { |
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.
Would use for (const __ of ___)
construct instead.
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.
[ts] Type 'Map<string, IStyle>' is not an array type or a string type.
packages/merge-styles/tsconfig.json
Outdated
"experimentalDecorators": true, | ||
"importHelpers": true, | ||
"noImplicitAny": true, | ||
"strictNullChecks": true, |
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.
+1
…, fix tests, remove async and appendChild features from stylesheet.
… usage. (microsoft#2527) * Initial checkin. * Updates to add webpack and slightly reduce the bundle size of merge-styles to 3.4k total. * Adding change files. * Adding more tests. * Updating version number. * Updates: remove ALL dependencies bringing the total size down to 2.7k, fix tests, remove async and appendChild features from stylesheet. * Fixing for loops and readme and strict mode. * const updates. * pr feedback. * Assume pixels when providing number types. * ignore opacity. * Adding keyframes test. * Adding comments, removing obsolete or deprecated properties from rawstyle interface. * Removing import. * Updating to optimize the transforms to an O(n) approach. * Updating documentation. * Minor grammar fixes.
This PR adds the merge-styles package, which will, in a future PR, allow us to remove the glamor reference. Please see the README included. Also see the tests to understand usage.
I have isolated the changes so that this PR only adds a new package. It is not used in any production code yet. This will come in other PRs, but I wanted to break out smaller reviews to ensure we focus on getting the main code completed and functional.
Office UI Fabric - merge-styles
The merge-styles library provides a number of utilities for loading styles through javascript. It is designed to make it simple to style components through javascript. It generates css rules, rather than using inline styling, to ensure we can use css features like pseudo selectors (:hover) and parent/child selectors (media queries).
The library was built for speed and size; the entire package is 2.7k gzipped.
The basic idea is to provide a method which can take in one or more style objects css styling javascript objects representing the styles for a given element, and return a single class name. If the same set of styling is passed in, the same name returns and nothing is re-registered.
This has a number of benefits over traditional build time staticly produced css:
Only register classes that are needed, when they're needed, reducing the overall selector count.
Dynamically create new class permutations based on contextual theming requirements. (Use a different theme inside of a DIV without downloading multiple copies of the css rule definitions.)
Use JavaScript to define the class content (using utilities like color converters, or reusing constant numbers becomes possible.)
Allow control libraries to merge customized styling in with their rules, avoiding complexities like css selector specificity.
Simplify RTL processing; lefts become rights in RTL, in the actual rules. No complexity like
html[dir=rtl]
prefixes necessary, which alleviates unexpected specificity bugs.Reduce bundle size. Automatically handles vendor prefixing, unit providing, RTL flipping, and margin/padding expansion (e.g. margin will automatically expand out to margin TRBL, so that we avoid specificity problems when merging things together.)
Reduce the build time overhead of running through CSS preprocessors.
TypeScript type safety; spell "background" wrong and get build breaks.
The api surfaces consists of TypeScript interfaces and a few important methods:
mergeStyles(..args[]
- Takes in one or more style objects, merges them in the right order, and produces a single css class name which can be injected into any component.mergeStyleSet
- Takes in one or more style set objects, each consisting of a set of areas, each which will produce a class name. Using this is analogous to calling mergeStyles for each property in the object, but ensures we maintain the set ordering when multiple style sets are merged.concatStyleSet
- In some cases you simply need to combine style sets, without actually generating class names (it is costs in performance to generate class names.) This tool returns a single set merging many together.Vocabulary
Let's clear up a few definitions before we start;
A style object represents the collection of css rules, except that the names are camelCased rather than kebab-cased. Example:
Additionally, style objects can contain selectors under the
selectors
property:A style set represents a map of area to style object. When building a component, you need to generate a class name for each element that requires styling. You would defint this in a style set.
Basic usage
When building a component, you will need a style set map of class names to inject into your elements' class attributes.
The recommended pattern is to provide the classnames in a separate function, typically in a separate file
ComponentName.classNames.ts
.The class map can then be used in a component:
Managing conditionals and states
Style objects can be represented by a simple object, but also can be an array of the objects. The merge functions will handle arrays and merge things together in the given order. They will also ignore falsey values, allowing you to conditionalize the results.
In the following example, the root class generated will be different depending on the
isToggled
state:Optimizing for performance
Resolving the class names on every render can be an unwanted expense especially in hot spots where things are rendered frequently. To optimize, we recommend 2 guidelines:
For your
getClassNames
function, flatten all input parameters into simple immutable values. This helps thememoize
utility to cache the results based on the input.Use the
memoize
function from the@uifabric/utilities
package to cache the results, given a unique combination of inputs. Example:Registering fonts
Registering font faces example:
Note that in cases like
fontFamily
you may need to embed quotes in the string as shown above.Registering keyframes
Registering animation keyframes example:
Server-side rendering
Example:
Caveats: