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

[styles] 100% test coverage #14566

Merged
merged 2 commits into from
Feb 19, 2019

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 17, 2019

We don't keep track of the test coverage for the @material-ui/styles package. This has allowed us to quickly ship the new style hook API. It's time to improve the test coverage.

Before
Coverage ~80%

After
capture d ecran 2019-02-18 a 17 42 23

After this pull request, I can look into #13607.


The withStyles() React dev tool overhead is down from 4 components to 2. Thank you React for shipping the hook API!

Before
capture d ecran 2019-02-18 a 17 49 04

After
capture d ecran 2019-02-18 a 17 45 48

We might get it down to 1 component as we improve the ref story of Material-UI.

@oliviertassinari oliviertassinari added test package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Feb 17, 2019
@oliviertassinari oliviertassinari force-pushed the styles-test-coverage branch 2 times, most recently from b0071f0 to 6738aff Compare February 18, 2019 09:38
@oliviertassinari oliviertassinari self-assigned this Feb 18, 2019
@oliviertassinari oliviertassinari added this to the v4 milestone Feb 18, 2019
@oliviertassinari oliviertassinari force-pushed the styles-test-coverage branch 10 times, most recently from 32c0b95 to b30e3ea Compare February 18, 2019 16:40
@@ -40,7 +40,7 @@ module.exports = [
name: 'The size of the @material-ui/styles modules',
webpack: true,
path: 'packages/material-ui-styles/build/index.js',
limit: '15.7 KB',
limit: '15.4 KB',
Copy link
Member Author

Choose a reason for hiding this comment

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

:)

@oliviertassinari oliviertassinari removed their assignment Feb 18, 2019
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Will need to take a closer look at the hook returned from makeStyles.

const defaultTheme = defaultThemeOption || noopTheme;

const meta = name || 'Hook';
const meta = name || metaOption || 'Hook';
Copy link
Member

Choose a reason for hiding this comment

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

What is the use case for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Commenting the diff would make two tests fail:

24 passing (2s)
  2 failing

  1) withStyles
       integration
         should run lifecycles with no theme:

      AssertionError: expected { root: 'Hook-root-vy1bts' } to deeply equal { root: 'Empty-root-vy1bts' }
      + expected - actual

       {
      -  "root": "Hook-root-vy1bts"
      +  "root": "Empty-root-vy1bts"
       }

      at Context.deepEqual (packages/material-ui-styles/src/withStyles.test.js:80:14)

  2) withStyles
       classname quality
         should use the displayName:

      AssertionError: expected 'Hook' to equal 'a'
      + expected - actual

      -Hook
      +a

      at Context.strictEqual (packages/material-ui-styles/src/withStyles.test.js:219:14)

withStyles uses the display name to provide a meta option.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it just override the name then? It seems like the name and meta option are doing the same.

Copy link
Member Author

@oliviertassinari oliviertassinari Feb 18, 2019

Choose a reason for hiding this comment

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

The name value is human picked. The meta is inferred. The name can have a strong implication on the generated class names in production, the meta is only used for improving the debugging experience.

Copy link
Member

Choose a reason for hiding this comment

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

Can we change the name of meta then? It's a pretty generic term. What speaks against using inferredName?

Copy link
Member Author

Choose a reason for hiding this comment

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

The meta comes from JSS: https://cssinjs.org/jss-api?v=v10.0.0-alpha.10#create-style-sheet. We can replace it with classNamePrefix in this case?

packages/material-ui-styles/src/useTheme.js Show resolved Hide resolved
@oliviertassinari oliviertassinari merged commit a82ab04 into mui:next Feb 19, 2019
@oliviertassinari oliviertassinari deleted the styles-test-coverage branch February 19, 2019 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants