Skip to content
OverviewSolutions
1
Comments
66

Shashi Lo

@shashiloAustin, TX1,345 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

I’m currently learning...

React, TypeScript, Vue, CSS Architecture Structure

Latest solutions

Latest comments

  • Shashi Lo•1,345
    @shashilo

    Great job Grace. I really enjoyed how thorough you were with accessibility. The H1 was a nice touch. I did find some things that I'd like to highlight:

    • The naming convention doesn't follow a true BEM naming. There looks to be nested block names inside elements. EX: c-showcase__card-title
    • I'm not sure why you're using EM units in c-showcase__card-btn when you already have REM set to 16px as the default. This will be a problem if you every nested this component.

    And now for the nit picky items:

    • The width and height of both desktop and mobile are off. Desktop more than mobile. On Desktop, because your solution is smaller, all of the margins and paddings do not match the design.
    • The content inside the card, space between image, title, body, and button are off.
    • The button padding is not accurate.
    • The card body content has an opacity of 0.9 right now, but the design looks much lighter.
    • I'm not sure what justifies the breakpoint at 680px. I'm used to using a common standard breakpoint < 768px which will accommodate for all mobile/tablet screens from 0 to 767px.
  • Shashi Lo•1,345
    @shashilo

    Hey Natalia. This looks pretty good, but I did notice some details that could improve your solution:

    • Paying attention to the details. What I mean is looking at the fonts, line height, shadows, spacing, etc. When you work with a designer, they will be very picky with you implementing it almost pixel perfect.
    • Because you are utilizing Bootstrap, look into the available components it comes with already. One example is the button.
    • The naming convention could improve. If you are follow BEM, I don't see a Block called out. card__component could be a block as card-component. Then anything that is an Element could follow.
    • The proceed and cancel links do not have hover states. This is because there is not a href for them.
  • Shashi Lo•1,345
    @shashilo

    Hey Richard. I can spot some was you can make this clean cleaner. Here's a list:

    • Usage of modern screen breakpoints. https://getbootstrap.com/docs/5.0/layout/breakpoints/
    • Do not use Id's. Usage of class names here is more ideal. This is a small widget, but when you get into larger scale projects, almost everything is shared or reused. Practicing this will help you down the road. https://css-tricks.com/the-difference-between-id-and-class/
    • Do not append div to CSS selectors. This makes it too specific and harder to override. Simple is best.
    • Using SASS/SCSS or CSS variables to set global variables on reused values like colors, font family, etc.
    Marked as helpful
  • Shashi Lo•1,345
    @shashilo

    Looks really close to the design Anna. The colors look great and the overall implementation is clean. Here's some nit picky things I saw that can improve:

    • When I over lay the design onto your implementation, the size is incorrect. The width is too and tall.
    • Remove the padding top/bottom from the <body>. This is making the window taller than it should be.
    • The icon spacing between the title is slightly off. But on mobile, it needs more work.
    • The breakpoint you have @ 760px should be 768px. Follow these breakpoints for modern screens. Also, if you use min-width instead, it will be a mobile first approach. https://getbootstrap.com/docs/5.1/layout/breakpoints/
    • On your buttons, add border: 2px solid transparent; to the regular state. This way when the hover state comes into play, it doesn't add an additional 4 pixels to the height and width making the elements shift.
    Marked as helpful
  • Shashi Lo•1,345
    @shashilo

    This is a great start Kevin. It definitely looks the part, but can use some touchup. Knowing how particular designers and some companies ware with being pixel perfect, here are some suggestions:

    • Download an image overlay plugin like PixelParrallel. It allows you to see where you're a little off in certain areas based on the design mock ups.

    • Modals background should be darker.

    • Modal options should allow the user to click the entire block to select the option instead of the radio button. This will increase UX and conversion.

    • You did a great job centering the modals!

    • Mobile modal starts in the center of the modal and not the top.

    • Mobile font size and line-height seems off. Definitely the main__wrapper.

    • Mobile menu should have a more subtle transition on open/close.

    • Mobile menu <a> should expand the entire width for it's click state. Increases UX/UI.

    • Make sure logo links to homepage.

    • Instead of using media queries to adjust the mobile screen, build your CSS mobile first. On a slow mobile phone, you do not want the user's device to work twice as hard to render styles. But on Desktop, the user is most likely using a fast internet connection and can handle the additional media queries.

    Marked as helpful
  • Shashi Lo•1,345
    @shashilo

    I'm just going to review the design and not the React code. Here's some feedback:

    • Instead of showing nothing as the homepage, choose a default and show that. Without a default, it makes the user feel like there's an error with the site. • The level of detail you put into the responsiveness is fantastic! • I don't see the benefit of using CSS Grid in the Header, but it looks good. • Instead of using margin's in the Stats_Item, use gap: 0.6875rem on Stats_Line. • For the main nav, I'd expand the click area to the entire header. It will increase the UX and less chances of the user not being able to click on the menu item. • The plant transitions could be better when navigating through overview, structure, and geology.

    Overall, the site works really well. Great job!

    Marked as helpful
View more comments