ApriyadiH / tbheroes

tbheroes
tbheroes.vercel.app
0 stars 0 forks source link

FE code review #2

Open hurricanenara opened 1 year ago

hurricanenara commented 1 year ago
  1. in-line styling should be avoided -- reason is that if we have styled components, but have in-line styles here and there, we end up scattering css/styling logic and this is detrimental to maintainability of the codebase. once our codebase grows and there were ever a styling bug, it would be very hard to locate an in-line style bug https://github.com/ApriyadiH/tbheroes/blob/d171036bb7ac1a03fb95adc7160e55316cbf2e74/src/components/General/Home.jsx#L51

  2. i think we could easily create a reusable component for these columns, that way we reduce the amount of code needed to render the same set of columns (DRY: Don't Repeat Yourself) https://github.com/ApriyadiH/tbheroes/blob/d171036bb7ac1a03fb95adc7160e55316cbf2e74/src/components/General/Home.jsx#L50-L79

  3. api keys like this should not be committed & should always be kept in the .env file! this key is probably a testing api key, so it's not as critical, but if you were to commit a production api key, another person could easily tamper with it, and use it for their benefit https://github.com/ApriyadiH/tbheroes/blob/d171036bb7ac1a03fb95adc7160e55316cbf2e74/src/components/General/Map.jsx#L28

  4. this project is still in process, so it's ok, but when we push our final product, let's make sure to remove all of the console.logs and commented out code https://github.com/ApriyadiH/tbheroes/blob/d171036bb7ac1a03fb95adc7160e55316cbf2e74/src/components/General/Navbar.jsx#L20

  5. it'll be beneficial to start wrapping all of your functions declared in a component around useCallback -- doing so will memoize the function and prevent any unnecessary re-evaluation of the code (for variables, we can use useMemo) -- have a read & see if you can apply this improvement throughout the codebase! 😄 https://github.com/ApriyadiH/tbheroes/blob/d171036bb7ac1a03fb95adc7160e55316cbf2e74/src/components/General/Navbar.jsx#L23-L27

  6. there's a ton of code here that could use a reusable component to increase readability! and as i mentioned previously, we should avoid in-line styling https://github.com/ApriyadiH/tbheroes/blob/d171036bb7ac1a03fb95adc7160e55316cbf2e74/src/components/General/Navbar.jsx#L51-L64

  7. it seems like we're mixing Bootstrap and Styled Components -- right now it may be manageable, but as the codebase grows, this will get very messy very quickly -- it's always best to choose and stick with one technology for that reason https://github.com/ApriyadiH/tbheroes/blob/d171036bb7ac1a03fb95adc7160e55316cbf2e74/src/components/General/Register.jsx#L37

  8. could you check if the outside div is needed? (in all of the components in /pages actually!) https://github.com/ApriyadiH/tbheroes/blob/d171036bb7ac1a03fb95adc7160e55316cbf2e74/src/pages/Homepage.js#L6-L8

  9. we can destructure the parameters in each function -- can be applied in other places as well, and great job so far in naming variables, they are very easy to understand https://github.com/ApriyadiH/tbheroes/blob/d171036bb7ac1a03fb95adc7160e55316cbf2e74/src/redux/modules/userSlice.js#L44

          // example
          logout: ({ username, email, isAdmin, isLoggdIn }, action) => {
            username = '';
            email = '';
            isAdmin = '';
            isLoggedIn = false;
           },
ApriyadiH commented 1 year ago

Thank you very much Miss Nara.