Skip to content
OverviewSolutions
45
Comments
151
Prabhash Ranjan
@besttlookk

All comments

  • Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    Hi, Nicely done. Following are some points i like to add:

    • There is no error-state for invalid-url.
    • It would be nice if you remove the error on focus. Add "onFocus" event listner on input to reset the error.
    • It is not responsive for all screen-size. Whenever you develop anything always consider all screen-size even if it is not asked for.

    Good luck,

    Happy Coding

    0
  • Kijana R.•110
    @NotKijanaSubmitted over 2 years ago

    I wanted to work on state management and conditional rendering. As always I made this with the idea that it could be scaled with relative ease. I do welcome any feedback on how I could further improve upon this project.

    Tip Calculator

    2
    Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    HI, Following are some points i like to add:

    • Use input of type "number". User should not be able to enter any other character.
    • Use flex-box to center the whole content w.r.t screen
    • Check your design for large input and large output. Currently on entering large input digit overlap the input-icon. Also on getting large output, result comes out of the card.
    • Give max limit to input and custom percent to avoid getting very large output.

    Good luck,

    Happy coding

    Marked as helpful
    1
  • Ricky•470
    @pyaetheiNSubmitted over 2 years ago

    Everything works great but in smaller screens, the side navbar is still overflowing even though I added overflow-x: hidden to the body tag... (swipe left on the screen to see overflowing side navbar)

    My trials:

    • adding a wrapper with overflow-x: hidden around the main tag

    Any feedback would be appreciated!

    Responsive intro section with dropdown navigation using flexbox

    2
    Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    Hi, Nice work! Everything looks great. Here are a few points I like to add:

    • On clicking overlay, sidemenu should close(for small screen)
    • Dropdown should also also while clicking outside(for large screen)
    • For large screen it would have looked good when the content were covering the whole view-port height.

    Good luck,

    Happy coding

    Marked as helpful
    1
  • Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    Hi, NIcely done. Just a few things I like to add.

    • Font size is bit too small for small screen.
    • Dropdown should close on clicking outside(espcially for large screen)

    I liked your menu for small screen and also menu-icon

    Good luck,

    Happy coding

    Marked as helpful
    0
  • King•310
    @king-oldmateSubmitted over 2 years ago

    What I found difficult was keeping track of all the code I had produced - lots of different class and ID names can be confusing. I'm not exactly sure what best practices are but I know what I did ain't it.

    I'm also not sure why the arrows aren't showing up. They did locally.

    Intro section with dropdown

    1
    Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    HI, I saw your code. Here are some points.

    • To reset the css setting you dont have to write all those tag name. Just add these lines in all of your future projects.
    *, *::after, *::before{
  magin:0;
  padding: 0;
  box-sizing: inherit;
}

html{
  box-sizing: border-box;
  font-size: 100% // I tho make it 62.5% so that 1rem = 10px
}

body{
  min-height:100vh;
  font-size: 16px;(use rem)
  color:
  backdround-color:
 
}
    • Rather than writting this
       margin-bottom: 15px;
    margin-top: 25px;

    you can write like this

     margin-block: 25px 15px;
    • When ever there is any logo/icon near nav item. `::after/ ::before pseduo selector" are best to add them. You can do it like this
    // html
 <li class="nav-item"><a href="#"><img src="images/icon-todo.svg"><span>Todo List<span></a></li>

// css
nav-item span::after{
  content: url(<location of arrow image>) 
  margin-left: 1rem;
  }
    • This i am not sure of but as much i can remember adding onclick on tag is not a good habbit. Better way is to get that element in JS and add eventListner on it.

    I have also completed this challenge. Here is the link

    https://intro-section-with-dropdown-navigation-phi.vercel.app/

    I hope i helped you in any way possible. Good luck.

    Happy coding

    Marked as helpful
    1
  • DanielK•440
    @DanK1368Submitted over 2 years ago

    Hi guys, Since I'm practicing React as my first framework and recently familiarized myself with styled components I wanted to put that into practice with this challenge.

    I'm hoping to hear some feedback on what I could have improved on/written more efficiently. :)

    Responsive Dropdown Navigation using REACT & Styled Components

    3
    Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    HI, Nice work! Here are some points i like to make

    • Its not responsive for all screen size. When ever develop anything ake a habbit of making responsive even if it is not asked for.
    • Dropdown menu should close when clicking outside the dropdown.

    I also made this using styled-component. Here is the link if you want any refernce

    https://intro-section-with-dropdown-navigation-phi.vercel.app/

    Feel free to drop your feedback.

    Good luck,

    Happy coding

    0
  • Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    hi, Great work. Below are some points i like to make:

    • Shadow is too harsh for dropdown. Shadow should be subtle.
    • Dropdown should close automatically when clicking outside.
    • Add hover effect for fropdown item(for large screen)
    • for large screen the hero section should cover whole view-port height. Gap below looks weird.
    • For mobile add overlay of dark color below menu. Currently you are using shadow. Also when clicking outside the menu(i.e on overlay) menu should close.
    • Use HTML semantic tags for better accessibility. Wrap the main content with <main> tag rather than <div>

    Good luck, Happy coding

    Marked as helpful
    0
  • Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    HI, Nice work, Below are some issues i like to point out:

    • Add 'cursor-pointer` for nav-items and dropdown item.
    • Try to use HTML semantic tag for better accessibility. Use <main> tag to wrap the main content rather than <div>.
    • Dropdown should close when clicking outside.
    • Dropdown should open/close on clicking whole nav item not just arrow.
    • Dropdown item comes first than comes the box. Whic does not look good.

    Good luck, Happy coding.

    1
  • Marley Reyna•510
    @MarleyReynaSubmitted over 2 years ago

    Let me know where I can improve. This is my first time using tailwind so it's a little messy but I think I'm getting the hang of it. Hopefully, next time will be much easier or at least more organized :)

    Responsive Page built with ReactJS and Tailwind

    1
    Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    HI, Nice work Following are some points i like to make:

    • z-index of dropdown is lower than heading.
    • Add hover effect for dropdown item.'
    • on some screen size , dropddown content comes out of the box.
    • the dropdown should be such that on clicking outside it should close automatically.
    • Add some padding in Y direction for main content. Currently it is touching the edge of the screen.

    Good Luck, Happy Coding

    Marked as helpful
    1
  • Chris Nowicki•140
    @chris-nowickiSubmitted over 2 years ago

    I had a difficult time with centering the QR Card horizontally and vertically. I am not sure if the max-width and max-height were necessary in my .card-container class:

    .card-container {
    max-width: 1440px;
    min-width: 375px;
    height: 100vh;
    display: flex;
    align-items: center;
    justify-content: center;
}

    Any and all feedback on my code is welcome!

    QR Code Component

    2
    Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    HI, About your prob. Transfer these lines to the body tag to center your container.

    body{
 height: 100vh;
    display: flex;
    align-items: center;
    justify-content: center;
}

    This will work.

    Good Luck,

    happy coding

    0
  • Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago

    Hi,

    • You have lots of HTML issue and Accessibility issue. Go through the report to remove those.
    • Currently im not able to downvote.
    • On clicking edit.Previous text should be prefilled in the textarea. Currently its not.
    • I am not able to add reply also
    • User should not vote on its own comment.

    Good luck

    Happy coding

    0
  • Prabhash Ranjan•2,540
    @besttlookkPosted over 2 years ago
    • As you are new to sass i would suggest using "@use/@forward" instead of "@import". In futrue @import will be deprecated.
    • Currently the content is not centered w.r.t the screen . Even tho you used flex to do that. You just missed one thing.
    body {
    display: flex;
    justify-content: center;
    align-items: center;
   min-height:100vh;   //this is very important
  }
    • Dont forget to reset the padding and margin for better control in your design. Try to add below code in every project you make. Add it at the top
    *, *::before, *::after{
  padding:0;
  margin:0;
  box-sizing:border-box
}
    • If you want to write well maintened code. Do consider the order in which you add the property in css. Here is a link you can refer:

    https://9elements.com/css-rule-order/

    Good luck,

    Happy Coding

    Marked as helpful
    0