Skip to content
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

[DISCUSS] Unifying our tooling for generating style code across Elastic #1656

Closed
snide opened this issue Mar 2, 2019 · 16 comments
Closed

[DISCUSS] Unifying our tooling for generating style code across Elastic #1656

snide opened this issue Mar 2, 2019 · 16 comments

Comments

@snide
Copy link
Contributor

snide commented Mar 2, 2019

Summary

Currently there are various ways to generate "css" code at Elastic used throughout our products. To my knowledge there are the following systems currently in play.

  • Sass compiled to CSS. This is the most common
  • Styled components. In use in Obs and Code teams
  • Less (almost entirely phased out, mostly just the bootstrap stuff)
  • Style tags in combo with individual JS vars
  • Imported styling through imported components (bootstrap js stuff)

All of these have differing sources of truth. It would be nice if we could all use the same global scope.

Main problems with how we do things today

  • Our design code drifts when we use different solutions. Mixins aren't in use in SC apps, and they often have to replicate them in JS to make them work.
  • It makes onboarding and documentation harder when we use different solutions.
  • The blob style of CSS delivery is bad for performance, bad for dependencies (see charts having EUI as a dependency) and bad for portability / cascade.
  • Theming is done separately per application with variation between each app

What we want as an end result

  • Everyone to use the same system with a shared global toolkit
  • Themability to be a minimal concern of the applications. We have a goal to get UI based theming available.
  • Selectors that can't conflict, but are still greppable in a dev environment
  • A more performant application that allows for smaller chunking and portability. Specifically, we'd love EUI to be ala cart. Currently the CSS blob is our biggest barrier here.

Proposal

We move to a CSS in JS system that allows for Sass to be written alongside the JS layer. Current proposals include moving to either https://github.com/gajus/babel-plugin-react-css-modules or https://github.com/4Catalyzer/astroturf

Why one of those systems seems like a decent path forward

  • They allow us to write reuse the hundreds of Sass files we have now
  • They allow us prefix selectors so we don't have conflict of scope
  • They give us a separation of concerns between the style layer and the logic layer that is useful for code review (Kibana design is able to watch *.sass, this is important to us and gives us some level of visibility)

The negative is primarily that the separation of concerns leaves the opportunity for dead CSS. Deleting a class in the JS means you have to find and delete that class in the CSS.

The other negative is that it goes against our "use what you want" mantras. However, this seems to be less a concern as we've proscribed "Typescript everything" and "React everything" over the past two years.

Why is Sass such a sticking point to the designers

Sass does alot. It's not just providing variables. It's over a decade old and the utility of the language itself is very powerful beyond the ways you're used to using it.

That out of the way. Not all of Kibana is React yet. Compiled CSS is still needed in certain places so Sass (even if used in CSS in JS primarily) gives us an escape hatch in those pesky places (It can still compile down or be imported into singleton css compiles). Second, the vast majority of our code is currently in Sass using EUI's Sass scope. The migration took us over a year, and outside of the blob nature of the CSS generated, we have no complaints with our Sass usage as tooling. It's battle tested, comes with everything you need out of the box (no importing from NPM for color manipulation or other design tooling) to do 99% of our styling needs. It's also already documented extremely thoroughly, and something we can assume new designers will be familiar with when we hire them.

The problems we have with the Sass are mostly in how we've compiled it. Things we hope to solve with a CSS in JS solution.

Also of note is that the entirety of the Sass global scope comes in one line in Kibana @include styling_constants. This is very powerful. With any JS solution, we'd never get something as portable because it would need to abibe by usual JS importing rules.

Why is global scope useful to designers

Usually when you read articles about CSS you hear a lot about removing global scope. This is valid for compilation and the actual CSS, but often the opposite is true when we talk about how we develop design (where we often import global scope on purpose). To maintain consistent design in a large product you need a fairly opinionated and importable global scope. Again, we're not talking about just variables here, we're talking about broad, heavy handed mixins that do everything from color contrast to form bordering to focus states to animation curves. None of these make sense to be abstracted to a JS "component" layer, especially considering we often pass multiple mixins onto single selectors. They are simply portable snippets of style code that make up the DNA of our shared styling. Often they are used together or in pieces across a broad range of products.

Why has Styled Components usage been difficult alongside EUI

Styled components as a platform is fine. It's problems are mostly in our usage, which may or may not be fixable, so here's the problems we've had in case people have good ideas.

  • Not being able to inspect in browser because of autogenerated IDs sucks.
  • It can't talk to EUI's global scope beyond the variables. This immediately makes it a pain for us to work with. Want to use tintOrShade or @include euiBottomShadow($euiColorPrimary, .2) and you have to remake it in JS.
  • Theming currently is handled differently app to app. The stuff written in Sass just works currently, even if it has that terrible footprint.
  • The usage app to app has varied quite a bit. Everyone writes it in their own style. I've seen declarations in the top of templates, in the bottom, using one-off names or linked names across components. In general I'd say it's been messier than the style code we're writing in EUI (and trying to pass down to Kibana). There is no documentation I'm aware of to provide guidelines for this.
  • The designer's can't really monitor it's usage. When we had to redesign Kibana I could pull a list of every file that needed to change and could watch changes on certain files. When everything is mixed it's just darkness unless we open the files up.
  • As an example in practice. Try building something that looks like an eui form component with only access to the variables in JSON. See how many of the mixins and functions would need to be rewritten. This should be common, because I imagine people often will want to extend their own "form like" controls specific to their apps. The functions and mixins in play are pretty intense because the forms have lots of animations and lots of states. Again, this could all be rewritten in JS, but it's so complicated, we definitely wouldn't want two versions of these kind of functions floating around.

Reasons it would be nice to use the same tools

  • It's very difficult to not break things downstream when making global decisions in EUI if people use different tools.
  • It's very hard to account for design drift
  • People commonly write code to do the same thing multiple ways.
  • We can document once

Other possible alternatives

  • We convert everything to styled components, rewrite the global layer of EUI to be completely JS based and figure out ways to get it working alongside our angular and static templates.
  • We move forward as is and let people manage their tooling themselves and be comfortable with drift and possibly give up on more robust theming. Breaks will happen along the way and that's OK. Autonomy is more important than consistency and the teams working off pattern live with some manner of upkeep.
@snide
Copy link
Contributor Author

snide commented Mar 2, 2019

@elastic/apm-ui @elastic/infrastructure-ui @elastic/cloud-ui @elastic/codesearch @zumwalt

I promised @sqren a meeting to discuss styled component usage in Kibana but figured it was something we could do async since we're all in different timezones. I've tried to write down some of my opinions about it, but I'm up for ideas. We have not started work on any of this stuff and only have a general directive so there's no immediate pressure to make a decision.

@bevacqua
Copy link
Contributor

bevacqua commented Mar 2, 2019

What's the argument against SASS all the things?

@sorenlouv
Copy link
Member

sorenlouv commented Mar 3, 2019

I think the move to CSS Modules is a great step in the right direction, since they introduce encapsulation and avoid the global scoping problems that plain css/less/sass suffers from.
Since several teams have already transitioned to other css-in-js solutions (mostly styled-components) that offer benefits similar to CSS Modules, as well as integrating well with React's component model, I think it's worth considering if it is possible for EUI to retain the support it has today for both approaches.

Demo application with CSS Modules and Styled-Components
The capabilities and performance of CSS Modules and Styled-Components are very similar but how they integrate into a codebase differs a bit. It would therefore be useful to see a simple demo plugin in Kibana using CSS Modules - this way it is much easier to get a feeling for how it is to work with. Here I'm thinking about conditional styling, intellisense, mixins, theming, linting for dead styles, integrating with testing libs etc. I can setup something similar for styled-components.

Q & A

Want to use tintOrShade or @include euiBottomShadow($euiColorPrimary, .2) and you have to remake it in JS.

True, and probably the biggest blocker for having multiple systems. AFAICT everybody in Kibana using SC has also embraced Polished which is a small library that offers the manipulations you mention. The naming does differ a bit, eg. instead of lightness($textColor) it is getLuminance(textColor). These inconsistencies can be confusing.

Not being able to inspect in browser because of autogenerated IDs sucks.

This should be easy to fix. There is an official babel plugin that solves exactly this (you are not the only one to be annoyed by this :) )
https://www.styled-components.com/docs/tooling#babel-plugin

Theming currently is handled differently app to app. The stuff written in Sass just works currently, even if it has that terrible footprint.

Good point. I like the way Infra approached this and I don't think it's a lot of work to setup (wrap your application in a theme provider and inject eui variables). But having something that Just Works™ is definitely appealing.

The usage app to app has varied quite a bit. (...) There is no documentation I'm aware of to provide guidelines for this.

Agree - I've also noticed this. I think it comes from SC being a new approach and none of us really knows what the ideal way to structure it is. If we choose to keep styled-components around we should collaborate cross-team and figure out some best practices that could make it into a set of guidelines/style-guide.

As an example in practice. Try building something that looks like an eui form component with only access to the variables in JSON. See how many of the mixins and functions would need to be rewritten. This should be common, because I imagine people often will want to extend their own "form like" controls specific to their apps. The functions and mixins in play are pretty intense because the forms have lots of animations and lots of states. Again, this could all be rewritten in JS, but it's so complicated, we definitely wouldn't want two versions of these kind of functions floating around.

This is similar to the point you made above with tintOrShade mixin (which I agree with). However in practice I don't see us recreating EUI components - wouldn't that defeat the purpose of EUI?
Mostly we align our UI requirements with the abilities of EUI. If a component doesn't support X we'll postpone X until it's in EUI (either by us adding it or someone from EUI team).

On a final note

I don't intent to come across as too dismissive to the idea of ditching SC in favor of CSS Modules. I'm mostly ok with it but want to make sure we are aligned with the pros/cons before teams spend a significant amount of time and resources refactoring their plugins to a new paradigm.

@snide
Copy link
Contributor Author

snide commented Mar 3, 2019

To your last point, don't worry, And don't assume my walls of text are me trying to force decisions either. I just come from a previous background of writing out specs and assumptions before committing to work. So to me it's just useful process :)

The one thing I'll mention about Polished, which is nice and recreates a lot of Sass functions is that it doesn't recreate the sass functions / mixins we create ourselves. tintOrShade and euiShadow being two of many dozens that we made to standardize things cross theme and cross component. I'm not worried about SC being able to recreate sass, which as you point out is pretty easy, I'm more worried about us making custom functions / mixins in multiple places (we'd have JS and Sass ones for each) that do the same things and drift over time.

@weltenwort
Copy link
Member

Thanks for compiling these arguments. I agree that a unification of the styling mechanism is highly desirable.

To me the biggest advantage of styled components is that styles can be located inline right alongside the presentational react components. This enables a separation of concerns (visual presentation as the result of entwined DOM structure and styles) instead of the "separation of technologies" as enforced by the JS/CSS file split.

From the feature list it looks like the astroturf library you mentioned can give us that feature as well as enabled the full re-use of the amazing work you put into the sass system. So 👍 to that from my side.

@snide
Copy link
Contributor Author

snide commented May 23, 2019

Summary from All hands discussion:

  • We need to unify. It's pretty important for us to all be using the same tech / codebase so we stay consistent.
  • We'll be moving to css modules or a similar system that supports Sass in EUI. We'll be moving everything in Kibana towards that goal. The EUI team is coming up with that solution during 7.3, with the hopes of converting everything by 8.
  • Till that time new projects in Kibana should be created with plain Sass and component scoping so that we're looking at a small conversion process when we make the switch to CSS modules or similar.

If you're not using Sass now in Kibana, I would recommend

  1. Start converting to plain old Sass using Kibana's current build tools. Componentize your sass next to your JS and follow EUI's formats.
  2. Use a three letter prefix for your app to avoid conflicts.
  3. When CSS modules (or similar) is introduced into Kibana's build process in 7.4ish, move your imports closer to the component and use that system. Your sass should still work.

You can wait on this process till after, but starting now gives us some manner of unity before this event happens and will make your conversion simpler later on.

I will set up a meeting and record it for all interested parties next week.

@pugnascotia
Copy link
Contributor

Was there representation from Cloud in that meeting?

@bevacqua
Copy link
Contributor

bevacqua commented May 24, 2019 via email

@snide
Copy link
Contributor Author

snide commented Jun 4, 2019

This week @thompsongl is starting proof of concept (likely multiple of them) work for this project. We'll set up a meeting in the following weeks and send a note to all of dev for review before we move forward. The earliest we'd anticipate movement here would be 7.4.

@streamich
Copy link
Contributor

streamich commented Jul 9, 2019

  1. Would really love to see EUI team pick some good CSS-in-JS solution like emotion, jss or nano-css.

  2. If you pick a CSS-in-JS solution, please choose one that does not just work with React, but one where you can define styles at module scope as well, like:

const buttonClass = rule({
	color: 'tomato',
	border: '1px solid orange'.
});

const Button = () => <button className={buttonClass}>Click me!</button>;
  1. One immediate problem I'm having now is that the New Platform in Kibana has no SASS support. As I understand from this thread you want to come up with a unified solution for everyone including Kibana, when can we expect to have a plan for SASS migration to the New Platform in Kibana?

    My preferred solution would be just to convert existing SASS that I am moving to the New Platform to emotion, which I can do for some plugins. (And it requires absolutely no setup, just adding emotion to package.json.) But for Kibana as a whole we need some plan of moving SASS to the New Platform. Maybe instead of concatenating that SASS into one bundle like in current Kibana, it could be some CSS module solution, where you import those SASS files from TypeScript?

@snide
Copy link
Contributor Author

snide commented Jul 9, 2019

@streamich Check the latest Kibana weekly for the latest status on the CSS-in-JS solution. There's a meeting recording as well as a gist linking out to some PoCs we're evaluating. We definitely don't want to come up with a third way of building CSS in Kibana, so I'd hold on adding something new there. Our plan is to send out a dev email / schedule a meeting for next week to the larger UI teams if you're interested.

One immediate problem I'm having now is that the New Platform in Kibana has no SASS support.

As to this. I'm guessing this is simply an oversight at the moment, but I'm not super knowledgable about that project and how Sass works there. I assumed it was the same as it works now. We have hundreds of Sass files in Kibana and they'll need to compile down since my guess is the platform conversion work and this CSS in JS work we're planning will both be gradual, separate projects.

Likely @epixa or @joshdover can fill in the gaps there.

@thompsongl
Copy link
Contributor

One immediate problem I'm having now is that the New Platform in Kibana has no SASS support

I haven't been able to find any references to Sass or scss in new platform locations. Relatedly, I haven't found anything in terms of documentation for styling in new platform.

@streamich is it just the lack of support at this time that raises questions about Sass in new platform, or is there documentation that suggests what styling (generally) will require?

@cchaos
Copy link
Contributor

cchaos commented Mar 16, 2020

We'll need to get back to working on this issue for the 7.8 time frame. In particular, the use case is being able to have components like EuiControlBar and EuiTooltip render as "dark theme in light mode".

@chandlerprall
Copy link
Contributor

render as "dark theme in light mode".

can you expand on that, or link to an existing definition/conversation?

@cchaos
Copy link
Contributor

cchaos commented Mar 16, 2020

This was sort of an internal note that @snide and I discussed. But we're seeing more components that render with dark backgrounds like EuiControlBar and EuiTooltip. Right now all their contents (aside from plain text which can easily inherit color), assumes they're in light theme and therefore won't render with proper contrast or visibility at all. The current terrible way to handle this is to have a class wrapper that imports the dark theme colors and re-exports all the components, doubling the compiled CSS which is no good. This really should be done at the component scope allowing for a "theme" context of variables to be passed and the appropriate styles be applied.

@cchaos
Copy link
Contributor

cchaos commented Aug 13, 2020

I think we may be done discussing and moving on to an actual plan 🎉

So I'm closing this issue in favor of #3912

@cchaos cchaos closed this as completed Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants