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

Migration to hooks #15231

Closed
28 of 29 tasks
joshwooding opened this issue Apr 6, 2019 · 46 comments · Fixed by #16693
Closed
28 of 29 tasks

Migration to hooks #15231

joshwooding opened this issue Apr 6, 2019 · 46 comments · Fixed by #16693
Labels
new feature New feature or request priority: important This change can make a difference umbrella For grouping multiple issues to provide a holistic view

Comments

@joshwooding
Copy link
Member

joshwooding commented Apr 6, 2019

We've made a great start in the conversion to Function Components. This is just an issue to keep track of what we class components (public and internal) we have left. I haven't checked if it's feasible to convert these components, if it's necessary or if it's worth it.

Core

Lab

Won't migrate

  • RootRef (soon deprecated)

Future work

The following items can be fully addressed once this effort is done:

@joshwooding joshwooding added new feature New feature or request package: material-ui Specific to @mui/material package: lab Specific to @mui/lab labels Apr 6, 2019
@oliviertassinari oliviertassinari added the umbrella For grouping multiple issues to provide a holistic view label Apr 7, 2019
@oliviertassinari

This comment has been minimized.

@eps1lon

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@eps1lon

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@eps1lon

This comment has been minimized.

@eps1lon
Copy link
Member

eps1lon commented Apr 9, 2019

@joshwooding If you don't have any priority for specific components it would be nice to prioritize Tooltip (improves RootRef -> useForkRef switch) and other components that use setRef.

@joshwooding
Copy link
Member Author

@joshwooding If you don't have any priority for specific components it would be nice to prioritize Tooltip (improves RootRef -> useForkRef switch) and other components that use setRef.

Luckily I've already done the majority of the Tooltip conversion :)

@oliviertassinari oliviertassinari changed the title Function Components Migration to hooks Apr 15, 2019
@oliviertassinari oliviertassinari removed package: material-ui Specific to @mui/material package: lab Specific to @mui/lab labels Apr 15, 2019
@oliviertassinari oliviertassinari pinned this issue Apr 15, 2019
@gautam-pahuja
Copy link
Contributor

Hi, @oliviertassinari I would like to try this for "Portal" component. Can I work on that if no one else has done it already? 😃

@oliviertassinari
Copy link
Member

Unless @joshwooding has already done an effort in this direction, you are good to go :).

@joshwooding
Copy link
Member Author

@gautam-relayr Feel free :)

@adeelibr
Copy link
Contributor

Can I work on the InputBase & Snackbar component? @oliviertassinari @joshwooding

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 17, 2019

@adeelibr Good for me :). For the Snackbar, I think that we should apply the same fix we applied to the Modal and Popper first. By the way, we still have a couple of demos using the class function, we need to migrate them too to the function component API :).

@adeelibr
Copy link
Contributor

For the Snackbar, I think that we should apply the same fix we applied to the Modal and Popper first.

Understood I'll have a look at the relevant PR's to understand what needs to be implemented for the Snackbar.

By the way, we still have a couple of demos using the class function, we need to migrate them too to the function component API :).

I must have missed them 😅 I'll have a look at it 👍

@austingardner
Copy link

Is the makeStyles API stable or is it subject to change? We currently use withStyles on an internal app here at WalmartLabs but we are eager to start using more hooks APIs.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 17, 2019

@austingardner I have looked into it in #15023. I haven't seen any issue (aside from the default props + API generation). We haven't moved forward as it's not a priority for the project right now. I only wanted to evaluate the potential. It should be ✅. I hope we can automate the conversion post v4. Maybe with a codemod?

@eps1lon has found an issue in StrictMode. The style sheets might not be removed from the DOM. Given withStyles uses makeStyles internally, everybody is impacted. So it shouldn't be a concern when choosing between withStyles and makeStyles.

By the way, we have added WalmartLabs logo in https://next.material-ui.com/. Let us know if it's OK for your team :).

@oliviertassinari
Copy link
Member

@austingardner To answer your question. No, we don't plan on changing the API of makeStyles.

@austingardner
Copy link

@oliviertassinari Thanks for the response! We'll start using it then.

@joshwooding
Copy link
Member Author

joshwooding commented Apr 18, 2019

Can I work on the InputBase & Snackbar component? @oliviertassinari @joshwooding

@adeelibr I'd prefer if people pick up one component at a time, but feel free to help. :)

@jacobbogers
Copy link
Contributor

ok, I analyzed your source code and use of npm module css-mediaquery used for SSR and mocha tests
I know now how to create the unit tests,

@oliviertassinari
Copy link
Member

I'm taking care of the Slider.

@jacobbogers
Copy link
Contributor

Some issues,
I am implementing this in a fork from master or should i have used next branch???

I put the hooks and contextProvider (for width) in the package material-ui-styles

running tests now, i get the following errors
Missing modules

  • expect.js
  • zen-observable
  • detect-browser

After installing all these modules with npm , (in the project root)

I run npm run test in directory /packages/material-ui-styles
i get the error

C:\Users\jacob\repos\ui\material-ui\packages\material-ui-styles\node_modules\jss-vendor-prefixer\lib\index.test.js:34
var isIE9 = _detectBrowser2['default'].name === 'ie' && _detectBrowser2['default'].version === '9.0.0';
                                       ^

TypeError: Cannot read property 'name' of undefined

@joshwooding
Copy link
Member Author

joshwooding commented May 11, 2019

@jacobbogers next should be used.

You need to use yarn to install the dependencies. We recently updated our documentation, but it looks like the note was only added in the docs/README.md. Sorry for the inconvenience.

@jacobbogers
Copy link
Contributor

Thanks, yes, I remember I came accorss this when I installed material-ui for the first time, sorry to have forgotten.

@jacobbogers
Copy link
Contributor

Can someone check out my PR so far?
Havent updated the docs yet, just wanted some feedback first #15678

@jeongsd
Copy link
Contributor

jeongsd commented May 14, 2019

Hi, Can I work on the ButtonBase component?

@joshwooding
Copy link
Member Author

@jeongsd ButtonBase is going to be one of the hardest components to migrate. I would start looking at SpeedDialAction instead

@adeelibr
Copy link
Contributor

Hi, Can I work on SwipeableDrawer?

@oliviertassinari
Copy link
Member

@adeelibr A previous effort started in #15469 (comment).

@adeelibr
Copy link
Contributor

Is SpeedDial Action available to work on? 😄 @oliviertassinari

@jeongsd
Copy link
Contributor

jeongsd commented May 15, 2019

Thanks for the guide @joshwooding, can I work on SpeedDial?

@bpas247
Copy link
Contributor

bpas247 commented Jul 22, 2019

Remove React.Component usage from the demos.

Is this task still up for grabs? I wouldn't mind taking it, if it's open 😃

@oliviertassinari
Copy link
Member

Unless @joshwooding has something in store for it. It's up for grab :).

@joshwooding
Copy link
Member Author

@bpas247 No plans here :) Feel free!

@bpas247
Copy link
Contributor

bpas247 commented Jul 22, 2019

Hmm looking at the doc examples right now, it looks like most of the component examples have already migrated to function components.

Are there any that haven’t? Or am I looking in the wrong directory for what the task is relevant to?

@oliviertassinari
Copy link
Member

This is one way to find them: https://github.com/mui-org/material-ui/search?l=JavaScript&p=1&q=%22React.Component%22.

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 priority: important This change can make a difference umbrella For grouping multiple issues to provide a holistic view
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants