This repository has been archived by the owner on Jun 26, 2020. It is now read-only.
Css modules #1236
Merged
Css modules #1236
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR gives me a "Spring cleaning" feeling. It was kind of annoying updating the styles, but once it was done it felt so nice. |
This enabled the theme prop to be removed from all three charting components.
|
I pushed one more example with da81414. This time I didn't use an external CSS file– but I did switch the inline styles for I'm not totally set on using CSS modules for styling here. I just think we should git rid of the theme context. |
|
I feel good about this. I don't think it's pressing enough to drop everything and rip out the context theme, but I think it's something that would be nice to do here and there, iteratively. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prototype of #1090. Builds on top of #1235 to migrate two files (
SettingsPaneandChartNode) away from using the "theme"contextand to using CSS variables instead.The
SettingsPanecomponent now uses CSS modules.ChartNodeon the other hand continues to use inline styles. I don't know if I have a strong preference for one approach over the other– I just want to get rid of the theme context and thought it would be worth trying both approaches.This PR shows a net increase in lines-of-code, but this is due to the
yarn.lockfile and additional CSS module Webpack loader config. As for the component code itself, so much code can be removed by doing this (compare before and after).I think we should gradually migrate views to use the new approach because it would dramatically simplify the process of updating and maintaining DevTools UI.
Incremental migration should be easy, since themes will continue to be passed down via context as well as written to CSS variables. (A nice end goal would be to remove the context entirely. 😄)