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] withStyles + forwardRef + hoist issue #13776

Closed
2 tasks done
amaslakov opened this issue Dec 3, 2018 · 12 comments
Closed
2 tasks done

[styles] withStyles + forwardRef + hoist issue #13776

amaslakov opened this issue Dec 3, 2018 · 12 comments
Labels
bug 🐛 Something doesn't work external dependency Blocked by external dependency, we can’t do anything about it priority: important This change can make a difference

Comments

@amaslakov
Copy link

Custom table pagination and Spanning table look strange:
image
image

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Steps to Reproduce 🕹

It can be found in the components demo section, Tabs.
I saw it when I cleared the cache. It reproduces in incognito mode.

Your Environment 🌎

Tech Version
Material-UI v3.6.1
React
Browser Chrome 70.0.3538.110 (Official Build) (64-bit), Firefox 63.0
TypeScript
etc.
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 4, 2018

@amaslakov I can't reproduce this. Could you provide more details on your platform? Does is still happen in private navigation?

@bmakan

This comment has been minimized.

@oliviertassinari
Copy link
Member

@bmakan The drawer problem on Firefox is tracked on a different issue.

@amaslakov
Copy link
Author

@amaslakov I can't reproduce this. Could you provide more details on your platform? Does is still happen in private navigation?

Yes. I see this in private mode in both Chrome and Firefox. I'm on Ubuntu 18.04
image
image
image
image

@oliviertassinari oliviertassinari changed the title [Docs] some Table examples don't look okay [styles] withStyles + forwardRef + hoist issue Dec 4, 2018
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Dec 4, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 4, 2018

@eps1lon 🤽‍♂️ the issue I was facing with https://github.com/mridgway/hoist-non-react-statics is back. Chaining two higher order components that use forwardRef and hoist-non-react-statics skip the render method of the last HOC 😱.
It's also a sign that nobody in the community is using forwardRef yet.
I'm patching the documentation to avoid the issue.

@oliviertassinari
Copy link
Member

@amaslakov Thank you for the repport 👌!

@oliviertassinari
Copy link
Member

The patch:

const CustomTableCell = withStyles(theme => ({
  head: {
    backgroundColor: theme.palette.common.black,
    color: theme.palette.common.white,
  },
  body: {
    fontSize: 14,
  },
-}))(TableCell);
+}))(props => <TableCell {...props} />);

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Dec 4, 2018
@oliviertassinari oliviertassinari added the external dependency Blocked by external dependency, we can’t do anything about it label Dec 4, 2018
@eps1lon
Copy link
Member

eps1lon commented Dec 5, 2018

Can reproduce it on Chrome Version 70.0.3538.110 (Official Build) (64-bit) and Firefox Quantum 63.0.3 (64-bit)

@oliviertassinari TableCell is a normal component. Why would this cause issues?

Edit:
This looks more like a deploy issue or className collision:
table-classname-black
table-classname-black-resolved

Deployed style and source style do not match:
https://github.com/mui-org/material-ui/blob/e5fbf74a06157e2d73701a158e8d6b5914429ea1/docs/src/pages/demos/tables/CustomPaginationActionsTable.js#L103-L106

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 5, 2018

@eps1lon We have the following configuration withStyle -> withStyles -> TableCell. The first withStyle render method is skipped (on the server or on the client, I don't remember). This leads to 1. missing CSS 2. rehydration issue (class name counter) between the server and the client 💣.

@eps1lon
Copy link
Member

eps1lon commented Dec 5, 2018

But hnrs should never override. It only hoists if a property does not exist on the target. The behavior is different between dev and production environment. hnrs does not care about the environment.

@oliviertassinari
Copy link
Member

I can reproduce the issue in development and production. Should I create a minimal reproduction?

@eps1lon
Copy link
Member

eps1lon commented Dec 5, 2018

@oliviertassinari That'd be nice. Although only so that I can understand it. #13818 should resolve hnrs issues for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work external dependency Blocked by external dependency, we can’t do anything about it priority: important This change can make a difference
Projects
None yet
Development

No branches or pull requests

4 participants