freeCodeCamp / chapter

A self-hosted event management tool for nonprofits
BSD 3-Clause "New" or "Revised" License
1.92k stars 360 forks source link

Remove MaterialUI #608

Closed Zeko369 closed 3 years ago

Zeko369 commented 3 years ago

Remove all Material UI components from our frontend and replace them with Chakra-UI

Arun-kc commented 3 years ago

Can I help? @Zeko369

Zeko369 commented 3 years ago

Go for it 🚀

Arun-kc commented 3 years ago

Hi @Zeko369, thanks for letting me help. I'm using the traditional dev environment. When I installed the dependencies it made changes in package-lock.json files. Will that be a problem as I need to commit these changes? What should I do about this?

Also the change I should make is just change material-ui to chakra-ui, right? Or will there be any other changes since both of them are different? 😅 I haven't worked with either of them yet. 😅

allella commented 3 years ago

@Arun-kc after which step is the package-lock.json changed? After the npx recursive-install ?

Also, which operating system and terminal / shell are you working with?

Arun-kc commented 3 years ago

@Arun-kc after which step is the package-lock.json changed? After the npx recursive-install ?

Also, which operating system and terminal / shell are you working with?

Hi @allella , yea the package-lock.json file changed when i did npx recursive-install for dependencies.

My OS is windows, I'm working with WSL in VS with Ubuntu 18.04 LTS

ojeytonwilliams commented 3 years ago

When I installed the dependencies it made changes in package-lock.json files. Will that be a problem as I need to commit these changes? What should I do about this?

Don't worry about the package-lock, it's automated by renovate-bot. If npx recursive-install generates new package-locks chances are you're using npm@6 (which is fine, btw!) and it's regenerating them.

TL;DR don't both committing any changes, the bot will take care of it.

Arun-kc commented 3 years ago

Don't worry about the package-lock, it's automated by renovate-bot. If npx recursive-install generates new package-locks chances are you're using npm@6 (which is fine, btw!) and it's regenerating them.

TL;DR don't both committing any changes, the bot will take care of it.

Thanks for clearing my doubt @ojeytonwilliams . I kept this work on hold because of this. I will resume my work on this soon.

ojeytonwilliams commented 3 years ago

No problem @Arun-kc. We're just a little unfortunate, because npm 7 is a relatively large change so not everyone has leapt on it.

The main thing is: don't feel like you have to do anything. If there are any issues, I'll resolve them.

ojeytonwilliams commented 3 years ago

Just to clarify, this does not need to be done in one PR. Small changes are very welcome.

SaNsK11 commented 3 years ago

Can I work on it?

allella commented 3 years ago

@SaNsK11 Yes, feel free to start small with a pull request to replace existing Material UI components with Chakra-UI.

More background on the reason for the change is shown in our meeting notes https://github.com/freeCodeCamp/chapter/wiki/Meeting-Notes-April-24,-2021

If you have questions, then please post on this issue or in the chat.

Reading the README and CONTRIBUTING.md are the best way to understand how the project and development is setup.

Ravichandra-C commented 3 years ago

@allella @ojeytonwilliams @Zeko369 When removing the material UI , I saw that in some places we have used makeStyles hook of material UI to create css classes? How should that be handled in chakra? Should we just pass them as Style props ? or Should we create a css Module for that component?

Zeko369 commented 3 years ago

@Ravichandra-C just pass them as Style Props

Ravichandra-C commented 3 years ago

@Zeko369 Thank you! I've made a PR with changes to ChapterForm and VenueForm Page components. Could you please check and let me know if they are ok?

Ravichandra-C commented 3 years ago

@Zeko369 How should this class be converted to chakra?

`pageRoot: {

  padding: `0 ${theme.spacing(8)}px`,
},
[theme.breakpoints.down('md')]: {
  padding: `0 ${theme.spacing(4)}px`,
},
[theme.breakpoints.down('sm')]: {
  padding: `0 ${theme.spacing(2)}px`,
},

}`

Its used in src/components/Pagelayout/index.tsx

ojeytonwilliams commented 3 years ago

Hey @Ravichandra-C did you figure this out in the end? Let me know if you'd like me to take a look.

Ravichandra-C commented 3 years ago

@ojeytonwilliams I passed them as style props.

pageRoot class is used in the client/src/components/PageLayout/index.tsx file like this

<div className={classes.pageRoot}>{children}</div>

I changed it to this

<Box pl={[4,4,8,16]} pr={[4,4,8,16]}>{children}</Box>

Everything is working perfectly, padding values are also correct except the actual breakpoints of materialUI and chakra UI are slightly different.

is that fine?

Ravichandra-C commented 3 years ago

@ojeytonwilliams @allella @Zeko369 On another note while removing the MaterialUI I noticed that these components are not referenced anywhere in code base. client\src\modules\dashboard\Venues\components\VenueItem.tsx client\src\modules\dashboard\Events\components\EventItem.tsx

Am I missing anything?

Should I still need to remove references of Material UI from them , if they are not referenced anywhere?

Zeko369 commented 3 years ago

@Ravichandra-C

{children}

This is great, I'd just change it to px instead of having pr and pl

Yup, remove VenueItem/EventItem if they're not imported anywhere

Ravichandra-C commented 3 years ago

@allella @ojeytonwilliams @Zeko369 Here is a list of pages/components that are not referenced anywhere.

Could I log another issue to remove these files? or Should I remove them as part of this Issue?

  1. client/src/components/PageLayout/pageLayout.styles.ts
  2. client/src/modules/dashboard/Events/components/EventItem.tsx
  3. client/src/modules/dashboard/Events/components/Tag.tsx
  4. client/src/modules/dashboard/shared/components/formStyles.ts
  5. client/src/modules/dashboard/Venues/components/VenueItem.tsx
  6. client/src/styles/theme.ts
Zeko369 commented 3 years ago

You can remove them in the existing PR

Ravichandra-C commented 3 years ago

Thank you! @Zeko369 . I've made a PR 704 with the above changes.