Skip to content
OverviewSolutions
1
Comments
4
Kyrie
@KyrieeWen

All comments

  • Mihail•160
    @mihail-soltanSubmitted almost 2 years ago

    Aligning all the containers perfectly got a bit challenging. Any suggestions are welcome

    Responsive news homepage

    1
    Kyrie•70
    @KyrieeWenPosted almost 2 years ago

    Hey MIHAIL, I reviewed your code, and think would be helpful to give you some feedback

    App.js,

    1. You can store those dummy data to another file called data.js for example to leave the functional component space cleaner..
    2. Good job to use nav to improve accessibility 3.navItems.map can be refactored to change i to item to make it more readable
    3. 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
    4. remove empty space on line 95, 97

    App.scss

    1. Good to use config file and store css variables to reuse
    2. would be good to seperate css and scss file, and store compiled css file to other folder
    3. 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
  • Kyrie•70
    @KyrieeWenPosted almost 2 years ago

    Hey Ogwang

    I think you deleted your code, could you please push again if need review? Thanks

    1
  • Kyrie•70
    @KyrieeWenPosted almost 2 years ago

    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
  • Kyrie•70
    @KyrieeWenPosted almost 2 years ago

    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