-
Notifications
You must be signed in to change notification settings - Fork 8
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
Rethink our css approach in the component libraries #112
Comments
I agree with your points, but I do think there's an important distinction to make. The way I see the problem we're solving with styled-jsx is shipping css with npm libraries. From my perspective that is different from handling css for apps (so CRA, etc.). When using regular .css with an app, I agree, there's not really a reason not to use regular .css files with css modules. But, shipping regular css with libraries is quite horrible in my experience. I think that that is the core problem that css-in-js solves for us here. And even though I would personally prefer a different solution, I don't know of a better way to ship css with an npm lib. than how we currently do it with css-in-js. So that's the argument for using css-in-js, vs. using regular css for our libraries. Now beyond that, there are different choices to make in which css-in-js library to choose. I have my personal preference there, but I believe the reason we went with styled-jsx is because it allows us to bake it into the lib, and doesn't require the user to include/use styled-jsx themselves. I think if we want to revisit these decisions, we should address the two requirements we set ourselves earlier:
(Unless we see a reason to revisit the requirements themselves) |
Please remember that this discussion is also not a pure problem that applies to this repository in isolation, so this discussion should not happen in this repo; it should go in https://github.com/dhis2/notes. There are other frontend libraries that produce components (notably https://github.com/dhis2/analytics) that have also committed to the decision we reached here: https://github.com/dhis2/notes/blob/master/decisions/2019/12/18-feds-test-location-attrs-css.md#what-to-use-for-css-styling-in-appscomponents-79 Decisions are not to be taken lightly, that is why the process surrounding them is designed the way it is. We commit to decisions as a group, even if we sometimes have to personally disagree and commit to serve the whole. |
(Moved to notes per Viktor's suggestion, to focus discussion on a general solution, vs just UI) |
In #108 @ismay documented a way to override styles of our ui components offered by @HendrikThePendric on slack. Maybe this is a good occasion to talk about our approach to css in our libraries.
We've chosen styled-jsx so users of our library don't have to handle css files during their build process. We've stuck to this approach for quite a while and I think it's time to talk about whether we think it's the best approach we can come up with or if there are more pragmatic approaches.
There are some issues I have with our current approach:
styled-jsx
adds a layer of complexity when having to differentiate betweencss
andresolve
resolve
function is a bit opaque in terms of how it should be used. There are times when I think I complete understood how to use it and then I run into an issue where things just don't work anymoreBut before we talk about
styled-jsx
itself, I think it's important to talk about the actual problem we're trying to solve and whether it actually is a problem or if we tried to preemptively solve an issue that might occur.The problems we're trying to handle
From what I can tell, there are two problems we're tackling:
We don't want users of our library to have to handle compiling css files.
Every modern webapp uses css
While this could be a nuisance, I think in reality this is not an issue whatsoever. I haven't encountered a single website / app in my entire career that doesn't use css and all modern webapps (at least the one using react) also handle css in some way.
App platform
At the same time, we're moving towards a unified approach how to create / architect dhis2 apps. We're doing this with several libraries, e. g. the app-platform. We're in complete control over how we think these apps should be built without having developers to worry about the internal structure if they don't want to. In fact create-react-app (which I think is used under the hood?) handles css already with css modules.
We're not trying to build a universal UI library
While we're trying to keep the
core
library as generic as possible so it could be used outside the dhis2 context, I don't think we should assume that this will be the case in the near future (the next ~2 years). If that is one of our goals we have to keep in mind that it'd be just another burden on the already relatively small dev teams and that there are already really good design system component libraries out there that we'd have to compete with (ant, kitten, mui, etc.)Conclusion
I think we're trying to solve a potential issue that won't be a real issue. By doing so we're introducing issues on the other hand for developers (see above). In my opinion we should consider that we might be causing more harm here than the issue we're trying to solve
We want to be able to style our components with regular css
I think this is a very valid problem as style objects are quite hard to work with when working on the components.
Use regular css when trying to use regular css
I think the best way to deal with css with by using
.css
files. It enables all the features in editors/IDEs, we'd be forced to have a clear separation between markup & styles by having a file for the component and a file for the component's styles.Overall conclusion
I think we should consider to both move away from
styled-jsx
and handle css extraction/compiling in the app platform.create-react-app
already supports css modules which has worked great for me in the past in apps. Most modern apps already support this approach as it's becoming more and more the standard to put css files next to the respective component file.The text was updated successfully, but these errors were encountered: