Skip to content
OverviewSolutions
1
Comments
3
ekando
@elliottkan

All comments

  • ekando•10
    @elliottkanPosted about 2 years ago

    Nice work!

    Noticed 2 minor things!

    1. Desktop image scaling issues when screen res is getting scaled down
    2. Just a nice little effect. Could add transition: ease-in-out 0.3ms' to your button` element

    Hope this helps! Perhaps you could check my project out and offer some feedback too!

    0
  • ekando•10
    @elliottkanPosted about 2 years ago

    Hey!

    Flexbox is definitely quite a tough beast to love! I like to think about breaking them down into small sized pieces for styling.

    1. I would add a max-width to the container to address the sizing issue instead of using margin.
    • You could also remove the div class container OR content since one of the divs are currently not doing anything, I would apply the background colour to the entire html element instead through *. By doing this, your .attribution should sit directly underneath your card.

    1. Play around with @media so the layout changes for responsive mobile design. You could then apply flex-direction: to address the change in layout.

    2. I believe it is best practice to use em as opposed to px for styling to cater for mobile first designs.

    3. Look at HTML semantics to understand better how to label your content, there are several h elements within your .wordings div. I believe the h4 could be replaced with <p> but I'm pretty new to this game still.

    4. could apply a width:100% to your img so it fits within container.

    I would recommend Kevin Powell to get some better understanding with CSS, he's an absolute weapon

    Hope this helps!

    0
  • ekando•10
    @elliottkanPosted about 2 years ago

    Hey Joseph,

    Looks good! I recently finished my own challenge recently!

    1. I've been told in the past that using em is preferable over rem or px. There seems to be some inconsistencies in that aspect in your code base.
    2. perfume currently lowercase.
    • Using the text-transform: uppercase; on your .tag could address some styling inconsistencies. Just an FYI incase this was a deliberate styling choice.
    1. The mobile layout seems to not have the photo loading issues but not sure if it's just me.
    2. Shop Button only easing in but not out.
    • I think applying transition: all 0.3s ease-in-out; to the button element will resolve the issue
    1. Ease-in-out elements are animating upon load.
    • Using a CSS reset would be good, this will remove the ease-in effects upon loading. CSS for animations.

    Good work! Hopefully you can have a look through my codebase and give me some feedback too!

    0