Mihail• 160
@mihail-soltan
Submitted
Aligning all the containers perfectly got a bit challenging. Any suggestions are welcome
Kyrie
@KyrieeWen
All comments
- Kyrie• 70
@KyrieeWen
Posted
Hey MIHAIL, I reviewed your code, and think would be helpful to give you some feedback
App.js,
- You can store those dummy data to another file called data.js for example to leave the functional component space cleaner..
- Good job to use nav to improve accessibility 3.navItems.map can be refactored to change i to item to make it more readable
- Consider to use BEM css name convention to name classes, nice to have 5.I would suggest to import image as xxx, then src={xxx}, in your case, each time when it re rendered, it needs to require those two images again 6.Change art to article to be more descriptive
- remove empty space on line 95, 97
App.scss
- Good to use config file and store css variables to reuse
- would be good to seperate css and scss file, and store compiled css file to other folder
- Good to use vw and rem
Let me know if there any specific question you want to ask me , happy to answer!.
Hope it would be helpful
Marked as helpful
0 - Ogwang Gift Gideon• 330
@Gito125
Submitted
Check out this site actually I developed it a year ago and I need your feedback.
Kyrie• 70@KyrieeWen
Posted
Hey Ogwang
I think you deleted your code, could you please push again if need review? Thanks
1 - wevs• 100
@dopewevmond
Submitted
Kyrie• 70@KyrieeWen
Posted
Hey wevs
Happy to see you finished this challenge, great work. I'd like to give some feedback after review your code.. hope it would be helpful
First of all, From App.js I'd like to recommend you read the react documentation of how to use useState hook. it should be const [a, setA] format, T is too ambiguous and hard to understand the actual meaning, and you can name it from togglerFunc to toggleTheme to match. Secondly, in Navbar.js, you can try to encapsulate those two components in different conditions to make your code easier to read.. Moreover, In CountriesPage, you can chain your fetch function with then make your response to be json format, and you can refactor it to not call api when its already loaded once (store data in local storage for example).. I saw you have commented eslint lines, you can configure them in eslint configure file and loading === false can be refactored to !loading, same for error.. In CountryDetailPage, you can rename cd to CountryDetails to make it more descriptive and then extract those properties out by doing .then({subregion, capital}) for example..
Generally speaking, really good project, achieved core functionality, used several hooks to make it easier to build.. Hope everything makes sense and can be helpful
Marked as helpful
0 - Okute Wonah• 150
@okutewonah
Submitted
Mobile version was kinda weird with the height. I had to increase the vh as the top part kept getting cut off the screen.
Kyrie• 70@KyrieeWen
Posted
Hey Okute, great work!
Good: 1. Define variables for primary colours 2. Descriptive comment to explain steps 3. Descriptive class name for 3 components
For the question you asked why need to set it to be 145 vh to not cut off the screen. it is because 100 vh means 100% of the current viewport height, so you set that body is 100vh tall including its padding and margin.. but the issue is on body's child, sedan, suv and luxury that you set padding 2.5rem but no height to be defined.. so overall body itself is 100vh with margin or padding, but it cannot control the sum of child's height and padding still be 100vh.. is that clear?
Also you dont need to use !important to override border radius that you set in 500px breakpoint, it still can be applied when screen size is <= 500px
Hope my feedback is helpful, keep going!
1
