Skip to content
OverviewSolutions
7
Comments
8
Bradley Smith
@bradleyhop

All comments

  • Bradley Smith•230
    @bradleyhop

    On the desktop view, your image doesn't quite extend to the bottom of the card container. Perhaps using min-height: 100%; on the <img> would work? Also, there's some flexbox styling applied to the image, but my browser tools show that its not being applied.

    The button's content also seems off: the cart is facing the wrong way and the text may be too small.

    Overall, excellent work! Small details (if you don't have access to the design files, don't sweat it), but the overall spirit of the design comes through. It was easy to read through your code, too. Inspiring.

    Marked as helpful
  • Bradley Smith•230
    @bradleyhop

    Excellent work! There are some small discrepancies in the mobile styling, for example the border-radius is 10px, the width is a little too wide, and some of the padding is different etc. (I was annoyed myself when I compared the desktop and mobile designs lol.) However, this implementation represents the design given well. I appreciate your use of CSS clamp(). I'm still using media-queries for most things. How do you determine your values? Mobile, tablet, and desktop values (in that order, of course)? Your solution inspired me to try this challenge for myself.

  • Bradley Smith•230
    @bradleyhop

    I think this is a difficult challenge with the backgrounds being one of the more irritating aspects to it haha. The image sizes supplied with this challenge don't match the design, so that makes it even harder. Anyway, look into the CSS property background-position and set the image to those provided using media-queries to link in the correct image for the device side. Don't forget to set the background-color on the same element as the background image (the image has transparency). See for more info:

    https://www.w3schools.com/cssref/pr_background-position.asp

    It also looks like the font for the buttons are incorrect. There seems to be some horizontal overflow, too.

    It looks great overall. Elements respond to device size, positioning is good, etc. Happy coding!

  • Bradley Smith•230
    @bradleyhop

    You're project matches the desktop design very well. Good job!

    I took a look at your code, and it looks like you only use on class per element, which leads to redundancy in your CSS. That, in turn, can make it harder to maintain your code as the projects become more complex. Let's take your cards, for example. In your html, you could do something like:

    <div class="card suvs">
  <p>Example content.</p>
</div>
<div class="card sedans">
  <p>More example content.</p>
</div>

    And then in your CSS:

    .card {
  height: 410px;
  width: 210px;
  padding: 45px;
}
.suvs {
  background-color: hsl(184, 100%, 22%);
}
.sedans {
  background-color: hsl(31, 77%, 52%);
}
/* etc */

    This way, you can set a base size for your card with one class and have another class that will specify specific attributes to that card. All the styling from multiple classes will be applied to element its assigned to.

    As far as making the site device-responsive, look into media queries and maybe start with CSS Flexbox to help you flow the cards into a column or row. A hot tip I picked up the other day: build the site with the styles being for mobile as the default, and then make changes as needed in your media queries (this is mobile first development and can really help you build your project).

    I've been learning lots from Kevin Powell's youtube channel for great tutorials and tips on CSS and other aspects of frontend web development: https://www.youtube.com/kepowob

    Happy coding!

    Marked as helpful
  • Bradley Smith•230
    @bradleyhop

    I agree -- I rebuilt this project twice. I really don't think it's a newbie project haha.

    I appreciate your question about CSS clamp() method. I actually didn't know about this feature and it led me to do some research. I usually set my widths in 'rem' measurements and make adjustments with media queries as I work up from smaller to larger devices when I code. clamp() seems dope, and I'll definitely give it try on my next project.

    I did run across this article about font sizes and clamp(): https://css-tricks.com/linearly-scale-font-size-with-css-clamp-based-on-the-viewport/#for-those-who-dont-mind-that-edge-case Under the "For those who don't mind edge cases" heading, the author has a generator for your fonts and clamp()!

    I like the animations on :hover. Nice touches. Also, I like the way you structured your SCSS files. I often feel lost when it comes to structuring a project, so it was nice to look browse your clearly written code.

    Marked as helpful
  • Bradley Smith•230
    @bradleyhop

    Since the social media icons are SVG format, I had trouble, too. I found this stackoverflow question: look at the pure CSS answer using the CSS "filter". There's even a link below that answer to help generate the color options you need using the CSS filter method.

    https://stackoverflow.com/questions/24933430/img-src-svg-changing-the-styles-with-css

    Also, consider wrapping your buttons and social media icons in anchor tags so that the pointer cursor appears when hovering per the design spec.

    General consensus on people that have completed this challenge is that this project is probably not actually a newbie level even though it's just HTML and CSS. Arranging responsively four different sections in the body is like whoa. Good job pushing through and completing it!

  • Bradley Smith•230
    @bradleyhop

    Your project look great! I took a look at your github repo and it seems pretty easy to navigate. I included the design files in my solution mainly because this is a learning exercise, and I wanted to show what I was working from. You could throw the design markdown file in the design folder to keep all that info together.

    I've done only one other Frontend Mentor project, but I'm paying for pro, so I downloaded the Figma design file. Since we're not supposed to upload those, I just used the readme.md template they provide to describe the project, etc.

    Your normalize.css is empty, so it's safe to delete. I'm not familiar with the normalize.css package, but as far as github pages is concerned, there's no information in that file to process. I only had one CSS file in my solution with just setting the margin and padding to 0, and box-sizing: border-box to clear out browser defaults.

    Marked as helpful
  • Bradley Smith•230
    @bradleyhop

    I found this article to be very helpful on centering: https://css-tricks.com/centering-css-complete-guide/ For my solution, I followed the guideline under centering vertically & horizontally, with the element of being a fixed width and height.

    I'd extend the width of your qr component card so that the text flows better. I do like how the color of the footer complements the color scheme. Great job overall!

    Marked as helpful