Skip to content

Conversation

@levadadenys
Copy link
Contributor

@levadadenys levadadenys commented Dec 13, 2022

Refactor after #397

No breaking changes, just refactor and optimization of repos codebase

The plan is to refactor React code step by step. (PR by PR)
In this PR I mainly separated the massive ui.jsx into separate components/files

This PR is not affecting #397 so can be merged anytime.

@levadadenys levadadenys mentioned this pull request Dec 13, 2022
@levadadenys levadadenys changed the title * refactor Refactor repo after updating to React v16 Dec 13, 2022
@levadadenys levadadenys requested a review from a team December 13, 2022 07:34
Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

this PR has a very large diff so i'm having difficulty understanding what optimizations were made and if there are any fixes related to the react upgrade. can you describe or leave comments around these specific changes?

most of this PR looks like clean-up work (mainly separating the massive ui.jsx file into separate files), so i want to make sure i'm reviewing this appropriately.

also, do you plan to merge this PR directly into main, or merge it into the branch in #396 first?

@levadadenys
Copy link
Contributor Author

this PR has a very large diff so i'm having difficulty understanding what optimizations were made and if there are any fixes related to the react upgrade. can you describe or leave comments around these specific changes?

most of this PR looks like clean-up work (mainly separating the massive ui.jsx file into separate files), so i want to make sure i'm reviewing this appropriately.

also, do you plan to merge this PR directly into main, or merge it into the branch in #396 first?

yeah, for now it's mostly cleanup work by separating the ui.jsx. The plan is refactoring step by step. First to separate the component so it would be either to navigate and understand the project and then do some more refactor. (Can be done in the third PR actually).

This pr works both on react v15 and v16 so I'm thinking of merging it into main once it's done. (By that time I think the react update will probably merged into main anyways).

@maddiedierker
Copy link
Contributor

thanks for confirming! that all makes sense. i'm interested in your thoughts on what we should refactor here, but if you've got a plan or an in-progress PR, you can just tag me on that.

@levadadenys levadadenys marked this pull request as ready for review December 13, 2022 21:51
@levadadenys
Copy link
Contributor Author

thanks for confirming! that all makes sense. i'm interested in your thoughts on what we should refactor here, but if you've got a plan or an in-progress PR, you can just tag me on that.

As for this PR I guess this might be it.
My next goals for next PR(s) would be:

  • fixing some console.warnings here and there (there's something to check in guide.jsx for sure, but I don't remember what exactly right now),
  • remove lodash where possible or update it's import not to import whole library at once
  • try to rewrite components as stateless functions where possible, cause using class based components might become an issue when updating to react 18+
  • (comes from the last point) think of an alternative to Radium lib (it doesn't allow to create stateless functional components)

@maddiedierker
Copy link
Contributor

that all makes sense. regarding this point:

think of an alternative to Radium lib (it doesn't allow to create stateless functional components)

we're moving away from Radium in the code-dot-org repo and replacing it with CSS modules (using SCSS syntax), so we would likely want to use the same styling solution here as well

@levadadenys
Copy link
Contributor Author

that all makes sense. regarding this point:

think of an alternative to Radium lib (it doesn't allow to create stateless functional components)

we're moving away from Radium in the code-dot-org repo and replacing it with CSS modules (using SCSS syntax), so we would likely want to use the same styling solution here as well

Ok, cool, thanks. Will use it!

Copy link
Member

@breville breville left a comment

Choose a reason for hiding this comment

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

This all looks reasonable. Thanks for taking an interest!

We must be careful to not regress any functionality. Does the tutorial operate the same through every user flow?

@levadadenys
Copy link
Contributor Author

This all looks reasonable. Thanks for taking an interest!

We must be careful to not regress any functionality. Does the tutorial operate the same through every user flow?

Yes, looks like everything works fine visually + test are passing successfuly

@levadadenys levadadenys merged commit 51eedd1 into main Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants