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

CSS-in-JS #101

Merged
merged 7 commits into from
Feb 12, 2021
Merged

CSS-in-JS #101

merged 7 commits into from
Feb 12, 2021

Conversation

omgovich
Copy link
Owner

@omgovich omgovich commented Dec 21, 2020

A lot of projects having trouble installing react-colorful because they do not have any CSS-loader.
Also, it blocks many developers from including our library as part of their public component libraries since most of them are 100% CSS-in-JS.

For example, I wanted to add react-colorful to react-three-gui but this project bundles via tsdx and do not support .css at all.

Solution: move the styles of our picker to JS.

@omgovich omgovich self-assigned this Dec 21, 2020
@omgovich omgovich mentioned this pull request Dec 21, 2020
@github-actions
Copy link

github-actions bot commented Dec 21, 2020

Size Change: -7 B (0%)

Total Size: 3.98 kB

Filename Size Change
./dist/index.css 0 B -687 B (removed) 🏆
dist/index.module.js 3.98 kB +680 B (+21%) 🚨

compressed-size-action

@omgovich omgovich marked this pull request as ready for review February 11, 2021 17:52
@omgovich omgovich requested a review from molefrog February 11, 2021 17:52
@omgovich
Copy link
Owner Author

@molefrog Can you take a look?

@omgovich
Copy link
Owner Author

@rschristian I would be glad to hear your opinion as well ❤️

@codecov-io
Copy link

Codecov Report

Merging #101 (a9abf22) into master (f84802a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #101   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           31        32    +1     
  Lines          402       411    +9     
  Branches        53        54    +1     
=========================================
+ Hits           402       411    +9     
Impacted Files Coverage Δ
src/components/common/AlphaColorPicker.tsx 100.00% <100.00%> (ø)
src/components/common/ColorPicker.tsx 100.00% <100.00%> (ø)
src/hooks/useStyleSheet.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f84802a...a9abf22. Read the comment docs.

@molefrog
Copy link
Collaborator

All looks good! Just a thought: if I decide to override the styles with my own ones, how will I be able to tell react-colorful to ignore the default styles? I mean I would have to deal with conflicting CSS in that case, right?

@omgovich
Copy link
Owner Author

Right. But, at the same point, the component's styles are too complex to write from scratch.

@molefrog
Copy link
Collaborator

@omgovich Yeah, you are right. I guess for the sake of backward-compatibility, the proposed solution is really good!

The only downside is that once you upgrade, you're going to see an error that the styles are missing. But imo, with a good migration message in the README, this might be doable.

@molefrog
Copy link
Collaborator

molefrog commented Feb 11, 2021

So I see two options here:

  • Publish a new major release
  • Don't remove the styles.css file yet in the next minor release, but make them redundant with a proper warning (will that be even possible?). And then remove the styles completely after everyone has migrated to the latest version.

@omgovich
Copy link
Owner Author

My plan is to release it as v5.

Copy link
Contributor

@rschristian rschristian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By all means, looks great to me.

@omgovich omgovich merged commit 0852864 into master Feb 12, 2021
@omgovich omgovich deleted the feature/css-in-js branch February 12, 2021 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants