Skip to content
OverviewSolutions
60
Comments
63
Fraser Watt
@fraserwat

All comments

  • Muhamad Nur Alfi Syahrin•100
    @TahoeBoelatSubmitted almost 3 years ago

    Hi everyone. This my submission for this challenge design. Overall i made using CSS Flexbox for the layout, but im curious how to make a left space in second and third rating or space on top in purple card section? For now im make a new class, and add the margin-left for the second and third rating, and margin-top for second and third card. Any ideas? Thank you

    Social proof section challenge solution

    1
    Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey! This is looking great!!

    Few things I'd change:

    • Semantic HTML: your <div class="container"> acts more like a <main>, and I would have the <header> and <main> sections you currently have both as <section>. Also id's tend to be used for javascript functionality, I would use classes for the labelling.
    • Gets a bit stretched out at larger screen widths. I would add max-width: 1100px; margin: auto; to the .container.

    Keep up the good work!! Fraser

    Marked as helpful
    1
  • Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey, this looks great!

    Few suggestions:

    • Check your <body> background colour against the one in the design. Should be one of the colors in the "README.md" file.
    • I'd have your @media query kick in a bit sooner, basically as soon as it doesn't "feel" right to have it in mobile/tablet mode anymore (its as much an art as a science)
    • I'd have max-widths as opposed to min-widths, the rest can be sorted out with padding
    • .CardContainer is redundant if you have a <main> element, that's your container.
    • Don't need to explicitly state margin-top if you're trying to vertically center your component. Try adding min-height: 100vh; display: flex; flex-direction: column to the body, then just using margin: auto to center your <main> component.

    Looks great though, keep up the good work!

    Fraser

    1
  • Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey Ozan, this is looking good!!

    I would remove the height attribute from card__body and add some padding to the bottom. At thinner screen widths this is causing issues with the text spilling out of the container. Most of the time you don't need to explicitly state the height and width for components (although max-width is often useful), and doing so often raises responsiveness issues.

    Keep up the good work!!

    0
  • Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey Jerome, this is looking nice!!

    Few suggestions:

    • Check out the font on the spec / readme, I think you're using a different one to the design
    • Take a look at the <span> documentation https://developer.mozilla.org/en-US/docs/Web/HTML/Element/span. Generally you use this for emphasising text (e.g. making a single word in a <p>tag<p> a different colour), or applying a different style to specific headers/paragraphs. You probably want to replace most of your span tags with a different html element.

    BEM stuff all seems to be in order, and your toggle is really smooth.

    Keep up the good work, its looking great!!

    Fraser

    Marked as helpful
    0
  • Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    This is looking great - I love the hover animation!!

    Couple of things I'd change:

    • If you have a min-height: 100vh (100% screen height) on the <body>, then you can use margin: auto on the .container to push your attribution element down to the bottom of the screen instead of being squashed up by your NFT card. You'll probably want to use a bit of padding-bottom on attribution to stop it being right at the bottom.
    • Think about what semantic HTML elements you can use to structure the page better. E.g. .attribution as a <footer>.
    • Change width to max-width and then use padding to fill out the gap between the edge of the component and the image. Especially when you've got lots of components on the screen, you don't want to be too explicit with widths and heights to make your page more responsive.

    Looks fantastic though, keep it up!

    Fraser

    Marked as helpful
    1
  • Nightcode16•110
    @Nightcode16Submitted almost 3 years ago

    Hello this is my solution on this challenge, I struggle on the background(on the two circle).Anyone can give advice and recommendation are welcome. Thank you!

    Profile Card Component using flexbox.

    2
    Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Looks like you're having trouble uploading your solution. Try this guide https://vercel.com/docs/concepts/git/vercel-for-github

    Marked as helpful
    0
  • Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey, this looks great!

    I would get rid of width in the .card div and replace it with a max-width, then you can have a more responsive solution, that just plays off padding you'll want to put on the body or container.

    Keep up the good work!

    Fraser

    Marked as helpful
    0
  • Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey - this is great!!

    Couple of things id change:

    • To make it more usable for people with screen readers etc, either add labels that aren't visible, or (probably easier) put in a aria-label="what the button does here" attribute into the HTML.
    • Change the cursor to a pointer over the calculator buttons - this better indicates interactivity to the user
    • I think the background-color on the first themes hover is too dark

    You could also look into what semantic HTML elements you could use instead of a div in .keys-container, but aside from that all looks great!

    Keep up the hard work!!

    Fraser

    Marked as helpful
    0
  • Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey, this is looking great!

    Few things I'd change:

    • Look into only needing a single <nav> component, and switching between it being a hamburger menu and an always-visible header menu depending on your media query. Here's a walkthrough https://dev.to/devggaurav/let-s-build-a-responsive-navbar-and-hamburger-menu-using-html-css-and-javascript-4gci
    • Instead of using height and width attributes, use max-width on your top-level container, and let everything else fill the screen responsively. Might need to play around with padding and margins a bit, but it will give you more flexibility, and you'll be able to build more responsive web pages.
    • The above will also make it easier to have more consistent design margins - e.g. the left and right edges of the heading don't line up with the left and right edges of your <main> component.

    Aside from that, great stuff - keep it up!!

    Fraser

    Marked as helpful
    0
  • Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey, this is really good!! Love the animations

    • As the .share-btn doesn't have any text, what you can do instead is use the aria-label attribute in HTML to describe what it does (i.e. "Open share menu"). This is good for accessibility, people with screen readers etc.

    Not really anything else I'd change. Great stuff!

    Marked as helpful
    1
  • Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey Morgan, this is looking really good!

    Few pointers:

    • As you have the HTML element <main>, you don't need class="main" and can just target everything in CSS with main { your rules here }.
    • Would take the margin-top and min-height out of the <main> element (generally want to avoid defining heights and widths as much as possible for responsiveness), have the min-height on the <body> instead, and then play around with auto margins for <main> and <footer> elements to center the main and move the footer down to the bottom.
    • If you take out the width CSS attributes from the <p> and .card elements, you can have a more responsive design, especially on mobile (you'll want to use padding on <body> to stop it going right up to the edges of the screen).

    Keep up the good work ,this is great!

    Fraser

    0
  • omar farid•370
    @omarfaridsSubmitted almost 3 years ago

    My first layout using JavaScript,it was tricky for me using JS code for the first time. still missing best practices but I managed to finish it as identical as i could. your feedback is so important to me.

    faq-accordion-card-main

    3
    Fraser Watt•1,790
    @fraserwatPosted almost 3 years ago

    Hey Omar! This is looking good! To get rid of those HTML issues, you can just have the test inside the <button> element without the <p> tag.

    Keep it up!

    Marked as helpful
    0