Skip to content
OverviewSolutions
1
Comments
66
Shashi Lo
@shashilo

All 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
  • Shashi Lo•1,345
    @shashilo

    Don't be discouraged by this, but let it be a way for you to learn. I'm particular with design reviews because you're going to get grind out by the designer for not paying attention to the details. I hope I'm not too harsh, and let me know if none of this makes sense. Cheers!

    Items to improve on:

    The details. You may not have the Sketch file, but if you overlay the design onto your implement or looked at it side by side, you'll see some differences.

    On Desktop View • Border-radius on the container is off • The content title and body should be left justified • Do not use <strong> unless it's made to be bold or stand out. For this instance, it's misleading because the entire title should be prominent. • Check the font sizes. They look a few sizes off on the title. • The stats column should be split into 3 equal parts and aligned to the start and end of the container. • The stats column label font and font size are incorrect. • The image should have an overlay fade. Right now it's way too bright.

    Responsiveness • If you look at your implementation at 900px to 1400px, the image is cut off. Instead, make the two sections equal in size and I'd start the mobile view below 768px.

    On Mobile view • The design should work on the minimum screen size of 375px wide. Why? Because this is the standard iPhone screen size nowadays. • All text should be aligned centered on mobile. • Please check the padding from the title to the body, from the body to the stats columns, and the stats columns to the bottom. It's all inconsistent and doesn't match the design.

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

    Good work here Kele. I know this challenge takes time to style and implement. There are some things you should clean up, but you should be proud of how far you've gotten.

    • I do like that you added a filter to the top of the page, but if I'm the designer or the product manager, I'd have you remove it until you click on a tag.
    • If you select multiple tags from the filter, it doesn't properly function.
    • On all your clickable states, there's no hover state. This is not good for usability as the user will not know if something is clickable.
    • If you look at the active-states.jpg, it tells you what should be clickable. Currently, you cannot click on the individual job posting tags. That's how a user will filter the results.
    • Font size, letter spacing, padding on tags, missing box-border on each listing, etc. These are details missed.
  • Shashi Lo•1,345
    @shashilo

    Hey Esraa. You're getting better at implementing designs, but I do see some areas that you can improve on:

    • The overall container should be max-width: 920px.
    • The method you are using to vertically and horizontally center the project is not ideal. It's not fluid and responsive. I'd remove this and make it margin: auto;. Because you're already using flexbox, add place-items: center;. This will center the elements vertically and horizontally.
    • In the left column, the icon box sizes are inconsistent.
    • Please check the font spacing, white bubble background color, details of the design. There are many little things off about this implementation. Please use PixelPerfect to overlay the design on top of your implementation to view the details I'm talking about.
    • The mobile view is pretty good aside from some padding issues.
    • Look into implementing your CSS mobile first.
  • Shashi Lo•1,345
    @shashilo

    Hey Cara. Looks like you were able to redo this using Flexbox. Great job! This implementation looks very much like the design too. I did find some areas that can improve:

    • .main should be 920px wide and the child containers should span the entire 920 width.
    • Remove the fixed height from .left-container. Allow this section to be fluid. This way if anything changed, it's easier to know where the fault is. If you inspect this element with browser dev tools, you'll see that the margin cuts the icons in half. Its true size is lost.
    • <div class="icon-bg"> has a hover state, but it's true clickable region is the <img>. For better usability, remove the div and move .icon-bg to <img class="icon-bg">. Less DOM elements, cleaner code, win-win.
    • .progress-bar-active should be 81.5% of the width.
    • Mobile looks great, but the left/right containers should have only 15px spacing between them.
  • Shashi Lo•1,345
    @shashilo

    Hey Jisha! You produced a good solution, however I found some things that can improved with your implementation:

    • Font-size and text color needs a closer look. You're setting your headings at font-weight: 500 when they should be 700.
    • Overall container border-radius and box-shadow is missing.
    • The submit button's border-radius and box-shadow are incorrect as well.
    • Your mobile implementation needs work. The body being width: 45% is not ideal. It should be 100% and add margin to the left/right to space the content from the edges.
  • Shashi Lo•1,345
    @shashilo

    Hey Shannon. You did a good job including all the elements and styles. I did find some items that can improve:

    • Overall, paying attention to the details. If you get PixelPerfect Chrome plugin, you can overlay the design and see where you are off from the design.
    • The Content padding is a little off.
    • Check the font colors, size, etc. There's a few places that it's a tad off.
    • You're missing the box-shadow from the main container and the submit button.
    • Mobile needs a little work. There needs to be space on the left/right of the container. Also, check the border-radius on Monthly Subscription box.
  • Shashi Lo•1,345
    @shashilo

    Hey Paminus. Great job getting this done. It looks good, but I did find some things you can improve on:

    • Overall width should be larger.
    • Font-size and text color needs a closer look.
    • Overall container box-shadow is also off. It's too harsh right now and should be more subtle and spread out.
    • The submit button's box-shadow are incorrect as well.
    • Check the Why Us content. It seems too big and needs more padding.
    • Check your margins on the mobile screen. Some elements need more spacing.
  • Shashi Lo•1,345
    @shashilo

    Very solid implementation Bhanu. It looks very close to the design, but I did find some areas of improvement:

    • Font-size and text color needs a closer look. You're <h2>'s are a little off.
    • Overall container border-radius is too small. The box-shadow is also off.
    • The submit button's border-radius and box-shadow are incorrect as well.

    Other than that, really good job!

  • Shashi Lo•1,345
    @shashilo

    Very unique implementation Romanoli. I don't know if this is the right place to use an input range as it's just for show and not an input element, but I like it! I believe there's ways to use pseudo elements to create the gradient, but you'd need to redo the current pseudo elements. The bubble should be a pure white color and I would make the entire icon div clickable for better usability.

  • Shashi Lo•1,345
    @shashilo

    Hey John. You did a great job using all the assets. It looks great, but it is missing some details. Here are the areas of improvement that I noticed:

    • You should create a .container with max-width: 1110px and used it within your sections.
    • Overall, paying attention to the detail of the design. For example, the hero section's title font size, call to action button's font size and alignment, etc. If you look at the design or overlay it onto your project, you'll see that you're close, but not pixel perfect to the design.
    • Instead of setting display: flex; to every main section, I'd use display: flex; on the parent and set flex-basis: % to create a width. You can also add align-items: center on the parent instead of each child element.
    • The box-shadow of the section's are too heavy.
    • Footer is missing the huddle image. Also, the content should start below the logo. Missing the social media icons as well. I know there isn't active states, but you should practice making all links real links.
  • Shashi Lo•1,345
    @shashilo

    Emmilie, this looks exactly like the design. I can't even tell the difference! Great job! I did find a few areas of improvement besides the border-radius.

    • The hover states should transition together. Currently, on desktop, when you hover on the CTA, the shared bubble transitions a little later. Onmouseleave, the button transitions quickly and before the shared button. There's no cohesion right now.
    • Missing content box-shadow.
    • The CTA doesn't work well on mobile. Either use an input button and make it pure CSS or use Javascript to toggle the states as the mobile user clicks on the CTA.
  • Shashi Lo•1,345
    @shashilo

    Really good job Jody, but I did find some room for improvement:

    • Background image is in wrong position.
    • max-width of content is too small. Should be 1110px.
    • Name and title should be inlined.
    • Image is missing border-radius. box-shadow is also too much on the image.
    • Position of the arrows vary depending on the viewport screen size. That's not ideal because it's not responsive. I'd position the arrow according to the image container so it's fluid with the design.