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

Support useStyles(style) for Hooks #159

Merged
merged 2 commits into from May 27, 2019
Merged

Conversation

piglovesyou
Copy link
Contributor

Fixes #154.

I'm glad if you take a look this, @frenzzy .

@codecov-io
Copy link

codecov-io commented May 11, 2019

Codecov Report

Merging #159 into master will increase coverage by 0.18%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
+ Coverage   80.76%   80.95%   +0.18%     
==========================================
  Files           3        4       +1     
  Lines          52       63      +11     
  Branches       12       15       +3     
==========================================
+ Hits           42       51       +9     
- Misses          9       11       +2     
  Partials        1        1
Impacted Files Coverage Δ
src/useStyles.js 81.81% <81.81%> (ø)

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 bd5af84...cc21210. Read the comment docs.

@piglovesyou
Copy link
Contributor Author

Glad if you take a look at this, @frenzzy .

@frenzzy
Copy link
Member

frenzzy commented May 21, 2019

I think all react-related stuff should be moved away from isomorphic-style-loader package.
Since webpack loaders usually are dev-dependencies but react HOC/hook must be a dependency.
See also #156

@piglovesyou
Copy link
Contributor Author

@frenzzy Thank you for the information, that's a good discussion. Yes, I'll follow decision the issue will make. Or, we can move withStyles and useStyles together later, since useStyles is just another implementation of withStyles.

@piglovesyou
Copy link
Contributor Author

@frenzzy Do you have a plan to separate the package into a loader and a react binding? That would be a breaking change. Would you like me to create the PR?

@frenzzy
Copy link
Member

frenzzy commented May 27, 2019

Or, we can move withStyles and useStyles together later, since useStyles is just another implementation of withStyles.

Agreed. Let's do this, I will release it as v5.1.0 today.

Do you have a plan to separate the package into a loader and a react binding? That would be a breaking change. Would you like me to create the PR?

It would be nice to split them, but I don't have time to do it. Need to create another repo for react bindings, configure builds, write tests etc. After that make a PR to isomorphic-style-loader with removed react stuff and special docs. Breaking change is not a problem I believe, we will just increase version number to v6. Help is really appreciated here, I even consider adding a new maintainers to this repo.

@frenzzy frenzzy merged commit c647f58 into kriasoft:master May 27, 2019
@frenzzy
Copy link
Member

frenzzy commented May 27, 2019

Released as v5.1.0 🎉

@piglovesyou
Copy link
Contributor Author

Thank you frenzzy. Yeah, that is going to be some work. I see, you're thinking another GitHub repo for it. I think I'll give it a try in the next free weekends.

@piglovesyou
Copy link
Contributor Author

@frenzzy Instead of separating the repo, what would you say if we about monorepo with Lerna?

Pros

  • Documenting in one place (root README.md) - Easy to read and easy to maintain
  • Easy to track for each packages' change for maintainers - Lerna has a functionality to link to each other
  • Possibly we can provide Vue.js binding as third package in the monorepo

Cons

  • Increase complication of codebase a bit - But this is inevitable when we separate it anyway
  • I'm still not sure Lerna is the right choice (Only two packages. Should it be worth for us?)

@piglovesyou
Copy link
Contributor Author

We finished this talk on #160 (comment)

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.

Hook?
3 participants