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

[withTheme] Fix typography warning #13707

Merged
merged 1 commit into from
Nov 27, 2018
Merged

Conversation

jmcpeak
Copy link
Contributor

@jmcpeak jmcpeak commented Nov 27, 2018

So that where core components create a default theme they don't receive the console warning.

Closes #13657

…te a default theme they don't receive the console warning.
@jmcpeak
Copy link
Contributor Author

jmcpeak commented Nov 27, 2018

Not sure how to test... ideas?

@jmcpeak jmcpeak changed the title [withTheme] Have withTheme match withStyles [styles/withTheme] Have withTheme match withStyles Nov 27, 2018
@oliviertassinari oliviertassinari added the new feature New feature or request label Nov 27, 2018
@eps1lon
Copy link
Member

eps1lon commented Nov 27, 2018

Every component creates a default theme. This is equivalent to disabling deprecation warnings by default. I don't think that's a good idea.

Originally the theme only displayed warnings if you modified a deprecated variant. This was changed so that it always warned if you use any mui component without setting typography to v2 to reduce bundle size.

Considering all the feedback and changes that were made my takeaway is that users do not appreciate deprecation warnings by default. Since this is also the approach react itself is taking (i.e. opt-in strict mode) I will consider this the next time I introduce a deprecation.

I would only wish that we settle on this in the RFC before I start the implementation. It's quite frustrating to think about good deprecation warning message if most of it is reverted without further discussion.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 27, 2018

I would only wish that we settle on this in the RFC before I start the implementation. It's quite frustrating to think about good deprecation warning message if most of it is reverted without further discussion.

@eps1lon The blame is on me, I have overlooked the RFC proposal. I'm sorry.

I agree with the takeaway, we should only warn for thing people are doing explicitly wrong. No for APIs that will change in the future. I think that this change is going in the right direction. Once we merge this pull request, we will warn when:

  • 👍 people use a deprecated typography variant with the component.
  • people create a new theme. We do it because people might directly rely on theme.typography.headline. Instead of a warning, we could conditionally warn when the theme value is accessed like Next.js is doing. However, it doesn't solve the body1 <-> body2 swap problem. The situation isn't perfect, but it's hard to find a better balance.

@oliviertassinari oliviertassinari changed the title [styles/withTheme] Have withTheme match withStyles [withTheme] Fix typography warning Nov 27, 2018
@oliviertassinari oliviertassinari merged commit 8fe2bfd into mui:master Nov 27, 2018
@oliviertassinari
Copy link
Member

@jmcpeak Thank you!

@jmcpeak
Copy link
Contributor Author

jmcpeak commented Nov 27, 2018

I'm not 100% sure why, but without the fix this simple create-react-app test produced a warning in the console:

(I believe because <TablePagination /> has a variant="caption" which exists in old and new)

import Adapter from 'enzyme-adapter-react-16';
import Enzyme, { mount } from 'enzyme';
import React from 'react';
import Table from '@material-ui/core/Table';
import TableFooter from '@material-ui/core/TableFooter';
import TablePagination from '@material-ui/core/TablePagination';
import TableRow from '@material-ui/core/TableRow';

Enzyme.configure({ adapter: new Adapter() });

mount(
  <Table>
    <TableFooter>
      <TableRow>
        <TablePagination
          count={0}
          onChangePage={() => {}}
          onChangeRowsPerPage={() => {}}
          page={0}
          rowsPerPage={0}
        />
      </TableRow>
    </TableFooter>
  </Table>
);

it('test', () => expect(true).toBe(true));

@eps1lon
Copy link
Member

eps1lon commented Nov 30, 2018

@jmcpeak What is the error message? We are not warning anymore if you use restyled variants.

@jmcpeak
Copy link
Contributor Author

jmcpeak commented Nov 30, 2018

@jmcpeak What is the error message? We are not warning anymore if you use restyled variants.

      Warning: Material-UI: you are using the deprecated typography variants that will be removed in the next major release.
      Please read the migration guide under https://material-ui.com/style/typography#migration-to-typography-v2```

@eps1lon
Copy link
Member

eps1lon commented Nov 30, 2018

Well this PR isn't released yet so this is expected.

@jmcpeak jmcpeak deleted the 13657 branch November 30, 2018 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants