microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.47k stars 2.73k forks source link

Foundational: Move to css modules. #520

Closed dzearing closed 7 years ago

dzearing commented 7 years ago

Recommendation:

Instead of outputing ms-Link class, we enable css modules to obfuscate the classes.

That way if a downstream dependency defines ms-Link, we don't collide. This also potentially enables side by side versioning where we have 2 copies of a component on the same page.

The downside is that it really removes some of the configurability of a component. For example, a ContextualMenuItem has a className, which gets set on the root of the menu item. Inside is an icon with the className ms-Icon. Today you can have selectors that target the guts of a component, like .myItem .ms-Icon, but with this change we would have to push these sorts of confurability into the props contract. (For example, expose iconProps, which gets mixed into the icon element.)

This may seem less robust, but it is safer. CSS class names are a really brittle contract.

pradeepna commented 7 years ago

@dzearing, this is going to impact us hard. What is the level of configurability we can expect here? We have overridden a few styles using class names now and we need to react to this at the earliest.

Regards, Pradeep

Jahnp commented 7 years ago

It would be great if we could identify some way to support both moduled and non-moduled CSS scenarios. There are significant pros and cons to each approach, and really address different audiences. While CSS classes are a brittle contract, they simply are the way deep visual UI configurability is achieved without overloading props for child elements/components.

dzearing commented 7 years ago

I am pretty convinced this is a bad idea. In talking with numerous partners, removing the classnames reduces a huge layer of configurability. I have discussed options with people, and it's really not clear. Even with inline styling, we'd need a much larger API surface to adequately reach the level of configurability. I think we should close this, or come up with an alternative plan to support major versions running side by side.

T-Hugs commented 7 years ago

@dzearing Does this mean that Fabric will treat class names as a public contract and maintain compatibility? Will there be any official guidance on customizing the guts of internal components, or will it be more of a "proceed at your own risk"? From our experience in VSTS, consumers will do this (and, indeed, VSTS has done it already), and it is very hard to maintain compatibility with all the crazy things people might do with CSS, but it also turns your DOM structure into a contract. We tried pretty hard to communicate that our CSS classes (in VSTS controls) and DOM structure should be treated as private/internal, and that we did not guarantee compatibility, but teams would still get upset when their stuff would break.

Edit: Just to clarify, I'm not saying it's a bad decision :) I just think there needs to be guidance around the usage of internal classnames and DOM structure.

dzearing commented 7 years ago

@trevorsg Sorry for the delay! There are a few nuances here.

First, we chose to completely keep all current classes intact, but only move the definitions to css modules. This solves the bleeding issue, versioning, while keeping customizations intact per @pradeepna concern above.

But long term, relying on flimsy class names is a terrible contract! We're asking for trouble. We need a contractual way to style things.

I see about 5 steps we need to work out to solve theming/customizability in a safe way:

  1. First a bit tangential; we lock down all TypeScript contracts with api extractor. This locks down the exports so that if you tweak something, you get a build warning asking you to re-run export. It means you need to do a manual step to change the shape of the exported TypeScript definitions. This helps keep us from accidentally removing something someone depends on. No more accidental build breaks due to api surface movement.

  2. The styling needs to be validated in the same way the code is validated. We want to move away from our existing css build process and move towards using glamor, which allows us to define rules in TypeScript, but have them injected within classes. This is close to inline styles without the completely rigid specificity issue. This means that we can define rules that can be contractually locked down. This has a side effect which is that the dev experience drastically improves. You get intellisense and red squiggles when you mess up. It also solves a bunch of other complex problems like css specificity, since all rules burn down to 1 level of specificity by default.

  3. Every component has an IComponentStyles interface, which defines the styling contract for the component. For example, imagine a BaseButton class defines the dom shape with an styles property typed as this interface. Now you can create a PrimaryButton variant that passes in one set of styles, and a CommandButton or ContextualMenuButton or NavButton that reuses the class and type-safe styling contract.

  4. For customization, every component has an IComponentTheme interface. This would contain the abstraction of customization knobs we allow you to define. Think backgroundColor. For button, this means we need to pipe that into the button element. If we implement split button, that one value would get piped into both buttons.

  5. We allow for theme injection through a ThemeZone component. This allows you to define a zone with a theme. The theme can override default properties for specific component types, allowing you to inject default overrides. But this isn't isolated to just colors or sizes, this would mean that, for example, maybe your "default" onRenderIcon renders an Icon component. But your app uses SVGs, so you could override the onRenderIcon for Icon component to nest an Image component that translates the icon name into a url. Crazy! Theme injection could actually allow you to make Personas render alternative implementations, without any modifications to the app code.

Well these are ideas, long term where we'd like to go. We definitely see 1-4 being realistic, and 5 being not that far off. We want to expose the right customization knobs without asking partners to rely on the internals (classnames, dom shape).

dzearing commented 7 years ago

The migration to css modules is complete so i'm closing this.