financial app code review

You may want to install prettier to format code automatically

then setup a commit hook like this to run in each commit/push/etc... https://github.com/FotonTech/golden-stack/blob/08935c5d145dbca2975a4ce7c7a77fe40de418df/package.json#L44-L56 also use: husky, lint-staged https://github.com/FotonTech/golden-stack/blob/master/husky.config.js

add --fix flag to auto fix issues https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/package.json#L10

I don't use imports like this anymore because it doesn't scale very well after a certain size. https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/src/pages/Cards.js#L6-L12 Consider using typescript with tsconfig paths setting, to avoid ../../ in imports, or use a babel plugin (maybe it's called root import I don't remember)

why did you choose to have one challenge repo instead of one repo per challenge? If I want to check out challenge A and clone it I end up cloning challenges B C D too

over-engineering: https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/src/pages/Cards.js#L52-L54 just add () => setIsEnabled((previousState) => !previousState) directly to the prop. You can actually do () => setIsEnabled(!isEnabled) should work about the same. The key idea here is to not add unnecessary stuff to the code, some things are just ugly and there's no fix to it, but the main problem is that "making the code nicer" is a very deep hole that will prevent you from shipping :)

cotainer and wrapper mean the same thing: https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/src/pages/Cards.js#L171-L172 maybe not the best names to use. Besides this, your variable naming is REALLY GOOD.

usage of responsive lib here is great: https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/src/pages/cards/styles.js#L5

I personally think that having a folder for styles like this: https://github.com/ildaneta/challenges/blob/master/financialApp/src/pages/cards/styles.js is just "hiding the code" https://twitter.com/thom_is_coding/status/1283433585249259521 if you just put it all into the same file, it would look like a lot of code... which leads me to the next point:

I think this looks good: https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/src/pages/Home.js#L7-L15 it's a well abstracted component with sub-components. I think this is too much JSX in the same place: https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/src/pages/Cards.js#L58-L193 you should break it down into components so it looks more like Home.js, and carry the styles together with the components, it makes sense to have them together because colocation is kinda good https://twitter.com/sseraphini/status/1121751206924378112

this isn't even a lot of styling: https://github.com/ildaneta/challenges/blob/master/financialApp/src/components/Main/styles.js ditch index.js and styles.js pattern and have them all in the same file, name the file Foo.js. It's better for you to find files and IDE too.

give components meaningful names to help debugging them: https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/src/components/Main/index.js#L32

version 16 has issues with multidex, it's safe to support 21 and up I think https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/android/build.gradle#L6

enable hermes :D https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/android/app/build.gradle#L81

add release signing config to buid signed apk for prod: https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/android/app/build.gradle#L146-L152 change this line then: https://github.com/ildaneta/challenges/blob/e847d5952d8c251d89d70685f2e64533c6c8517e/financialApp/android/app/build.gradle#L161

ternaries like this are ok, but I don't think they're the best solution

width: ${(props) => (props.menuCenter ? `${wp('13%')};` : `${wp('15%')};`)};
  height: ${(props) => (props.menuCenter ? `${hp('7')};` : `${hp('5%')};`)};
  margin-bottom: ${wp('2')};
  background: ${(props) => (props.menuCenter ? '#234F9D' : '#eceef2')};
  border-radius: ${(props) =>

It's a hard problem, but at some point in the furure maybe if need stuff like this, you need a component :thinking:

Keep up the good work ilda :D really nice to have people like you in the community

ildaneta / challenges

financial app code review #13