search

cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.08k stars 399 forks source link

Refactor withStyles #1507

Closed behnammodi closed 3 years ago

behnammodi commented 3 years ago

@kof I did but I need your help, Items:

SUMMARY of Test: 713 tests completed 12 tests skipped 2 tests failed

kof commented 3 years ago

One of the errors from tests is the missing props.theming handling 

  React-JSS: theming withStyles()
    when theming object returned from createTheming is provided to injectSheet options
      ✖ allows nested ThemeProviders with custom namespace
        Chrome 90.0.4430 (Mac OS X 10.15.7)
      Error: expected undefined to sort of equal { color: '#aaa' }
          at Assertion.101.Assertion.assert (webpack://jss/node_modules/expect.js/index.js:96:0 <- packages/react-jss/tests/withStylesTheming.js:197:13)
          at Assertion.101.Assertion.eql (webpack://jss/node_modules/expect.js/index.js:230:0 <- packages/react-jss/tests/withStylesTheming.js:331:10)
          at Context.<anonymous> (webpack://jss/packages/react-jss/tests/withStylesTheming.js:400:43 <- packages/react-jss/tests/withStylesTheming.js:20995:86)
behnammodi commented 3 years ago

One of the errors from tests is the missing props.theming handling

  React-JSS: theming withStyles()
    when theming object returned from createTheming is provided to injectSheet options
      ✖ allows nested ThemeProviders with custom namespace
        Chrome 90.0.4430 (Mac OS X 10.15.7)
      Error: expected undefined to sort of equal { color: '#aaa' }
          at Assertion.101.Assertion.assert (webpack://jss/node_modules/expect.js/index.js:96:0 <- packages/react-jss/tests/withStylesTheming.js:197:13)
          at Assertion.101.Assertion.eql (webpack://jss/node_modules/expect.js/index.js:230:0 <- packages/react-jss/tests/withStylesTheming.js:331:10)
          at Context.<anonymous> (webpack://jss/packages/react-jss/tests/withStylesTheming.js:400:43 <- packages/react-jss/tests/withStylesTheming.js:20995:86)

Could you advise?

kof commented 3 years ago

when props.theming is received you need to handle that and use it instead of the theming from context, see the test

kof commented 3 years ago

Just looked at it more closely, I was wrong about props, we don't receive theming over props, only theme, so its in options. Your mistake was not using theming.context

behnammodi commented 3 years ago

Just looked at it more closely, I was wrong about props, we don't receive theming over props, only theme, so its in options. Your mistake was not using theming.context

Yes, I add theming.context but it does not work again

kof commented 3 years ago

Yeas, I add theming.context but it not work again

There is another bug somewhere, it passed the first 2 assertions

      expect(themeReceivedInComponentA).to.eql(themeA)
      expect(themeReceivedInComponentB).to.eql(themeB)

Creating a PR

kof commented 3 years ago

https://github.com/cssinjs/jss/pull/1508

see last commit

behnammodi commented 3 years ago

I fixed

kof commented 3 years ago

I think you have permissions to commit directly into that new PR

behnammodi commented 3 years ago

I think you have permissions to commit directly into that new PR

I haven't permissions, but anyway I fixed on this PR

behnammodi commented 3 years ago

I think you have permissions to commit directly into that new PR

Also, I apply your changes

kof commented 3 years ago

I changed the target branch of this PR so that I can commit somewhere and we can merge it. Please rebase this PR.

In the last commit I fixed the second mistake, createUseStyles() is accepting theming, not theme. This has fixed that failing test now.

behnammodi commented 3 years ago

I changed the target branch of this PR so that I can commit somewhere and we can merge it. Please rebase this PR.

In the last commit I fixed the second mistake, createUseStyles() is accepting theming, not theme. This has fixed that failing test now.

I fixed too

behnammodi commented 3 years ago

@kof We just have 1 error in test cases

should render a number of composed style rules with withStyles API

kof commented 3 years ago

@kof We just have 1 error in test cases

yeah, I am confident you can figure it out

behnammodi commented 3 years ago

@kof Thanks man, I hope :)

behnammodi commented 3 years ago

@kof We don't have any corresponding test case about should render a number of composed style rules with styled API for useStyles so I'm completely confusing. I find a piece of code that is not inside createUseStyles but we have it on previous withStyles:

if (sheet && registry) {
  registry.add(sheet)
}

And I don't know we need it or no

kof commented 3 years ago

@behnammodi not sure what you are confused about, from what I can see we have the register.add() call inside manageSheet()

behnammodi commented 3 years ago

@behnammodi not sure what you are confused about, from what I can see we have the register.add() call inside manageSheet()

Yes, you're right 👍🏻

kof commented 3 years ago

@behnammodi I am assuming you will let me know if you are completely stuck with this

behnammodi commented 3 years ago

@behnammodi I am assuming you will let me know if you are completely stuck with this

Yes, In the last test case I'm completely stuck

kof commented 3 years ago

ok, going to look into it

behnammodi commented 3 years ago

ok, going to look into it

Awesome 👍🏻

kof commented 3 years ago

Fixed, please rebase

kof commented 3 years ago

A bunch of type errors still there, otherwise all tests passed

behnammodi commented 3 years ago

Fixed, please rebase

Thanks man 👍🏻 🙏🏻

behnammodi commented 3 years ago

@kof Hi man, I fixed temporarily flow issue, so you can review it again

behnammodi commented 3 years ago

@kof I had a very busy week 🙏🏻

kof commented 3 years ago

merged, thank you!

kof commented 3 years ago

released in https://github.com/cssinjs/jss/releases/tag/v10.7.0