Countries: React, redux-toolkit with RTK Query, change theme

Community feedback

• 

  j0sephh123•140

  @j0sephh123Posted about 2 years ago

  • Why create-react-app ? It is perfectly fine, just curious.
  • .map<React.ReactNode> why typing that? It is inferred by TS compiler.
  • Noticed that in some components you are importing React, I don't think it is necessary

  src/copmonents/pageLayout/index.tsx

  • you are defining the typed hooks from redux docs in here src/app/hooks.ts, but you are still using the original ones
  • for theme you have type string, here it'd be a good place to use enum or union type, since it can be only 2 values
  • onChangeTheme function is doing some logic inside the component dispatch(setTheme(theme !== "light" ? "light" : "dark")); I would suggest that you actually do that logic inside the reducer.

  src/pages/home/index.tsx

  • dataAll and allLoading are not good names for variables.
  • byRegionLoading is unused variable
  • you could great benefit from es-lint, it gives me a few warning, such as not returning a value from filter function, React Hook useEffect has a missing dependency: 'closeOpenMenus'. Either include it or remove the dependency array.
  • you would benefit from creating components for Error and Loading.
  • there is a lot of repetition in result.map, you could do some destructuring there.
  • Since const data: CountryApi | undefined it would be good to handle the fetching in one component and only have a child component that receives data when we are certain it is only CountryApi. Thus, you can avoid checking on severa places if it is undefined.
  • probably can extract hideSelect && what will be here in a custom component, it would need to take only one prop handleChangeRegion, since the listSelect is a hard coded array.
  • I think that you can type region to a union type, currently it is a string. You can have all the possible values here: src/pages/home/config.ts

  src/copmonents/card/index.tsx

  • capital && you are checking for capital truthiness here, but even if it is an empty array, it is still truthy, so there is no need for that check. A better approach would be to check its length.

  src/@types/index.ts

  • you could add the type for capital also

  src/pages/country/index.tsx

  • See here you are using again Loading and Error. You are repeating yourself - extract them as components
  • Same note for country here. Create a child component, check if country is NOT undefined, then pass it to the child component, thus you will avoid all the ? checks that you have for country.
  • why is CountriesCode CapitalCase?

  Marked as helpful

  2

  ---

  

  Yuriy•155

  @faizovPosted about 2 years ago

  @j0sephh123 Thanks for the extensive advice! I'll try to make corrections soon.

  I don't quite understand about the first question. What other options are there?) I want to learn react completely.

  In case of .map<React.ReactNode> I get an error ts

  0

  

  j0sephh123•140

  @j0sephh123Posted about 2 years ago

  @faizov I suggest you look into https://nextjs.org/docs/getting-started

  0

  

  Yuriy•155

  @faizovPosted about 2 years ago

  @j0sephh123 Thanks!

  0