Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Log in with GitHub
OverviewSolutions60Comments63

All comments

  • Muhamad Nur Alfi Syahrin• 100

    @TahoeBoelat

    Submitted

    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

    @fraserwat

    Posted

    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

    @fraserwat

    Posted

    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

    @fraserwat

    Posted

    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

    @fraserwat

    Posted

    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

    @fraserwat

    Posted

    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

    @Nightcode16

    Submitted

    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

    @fraserwat

    Posted

    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

    @fraserwat

    Posted

    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

    @fraserwat

    Posted

    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

    @fraserwat

    Posted

    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

    @fraserwat

    Posted

    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

    @fraserwat

    Posted

    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

    @omarfarids

    Submitted

    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

    @fraserwat

    Posted

    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
  • Fraser Watt• 1,790

    @fraserwat

    Posted

    Hey Gloria! Form validation seems to work ok for me! You'd have the actual submission in the <form> html, but that's beyond what this challenge is asking.

    One thing I would change about it though is have the validation-text be set to default as display: none instead of visibility: 0. This way, its appearance on the screen will be shown by screen readers.

    Would improve this by thinking semantically (good for SEO and accessibility) - for example what I mean by this. your <div class="logo"> element could be a <header>, etc.

    Keep it up!

    Marked as helpful

    1
  • chief• 220

    @cujothechief

    Submitted

    hey there. uh, this is my solution to the article preview component challenge. I had a few challenges making the image fit its div and also a little problem making the responsiveness a bit more seamless. feel free to look through my code: suggestions and constructive criticism is welcomed. thank you! <3

    responsive article preview component

    1

    Fraser Watt• 1,790

    @fraserwat

    Posted

    Hey, good stuff!

    Would wrap the button element for the pop out menu in a <button> element. Good for accessibility, you can tab to it and it'll allow you to put a background on it.

    Also look into how you can use padding to avoid having to set explicit widths. Especially when you have loads of components on the same screen, you want to be stating the exact heights and widths of things as little as possible so that everything can be responsive at different screen widths.

    e.g. the "text" div (i'd have this as a class, not an id btw), if you set left and right padding here you can get rid of the explicit width and margin attributes on the header and p elements.

    Keep up the good work!

    Fraser

    Marked as helpful

    0
  • Fraser Watt• 1,790

    @fraserwat

    Posted

    Perfect!! The only thing I'd change is maybe check out the font sizes at larger page widths (maybe even try clamp() if you're feeling adventurous!)

    Marked as helpful

    1
  • Fraser Watt• 1,790

    @fraserwat

    Posted

    Hey Pavan, this is looking good - think you could make a few changes to get this looking perfect.

    • Get rid of <div class="body">, and all the CSS rules on the <body> tag, put the background-color from <div class="body"> onto <body>
    • Get rid of height attributes in your divs and change all width CSS attributes to max-width - doesn't matter as much in this challenge but when we've got loads of components on screen we don't want to be strict with dimensions to be as responsive as possible.
    • Use width: 100% on the img - this way it fills its container, but it also doesn't matter if you have a screen smaller than the width you've coded in.
    • Change padding-top to just padding in .code, so you have even boundaries around your content

    That should do the trick - keep up the good work!!

    Fraser

    Marked as helpful

    0
  • Fraser Watt• 1,790

    @fraserwat

    Posted

    Hey ,this looks great!!

    Couple of changes I'd make -

    • On mobile view, the attribution footer element moves to the top of the screen? Also if you have it in a footer you can just give the footer a class of attribution, and get rid of the .attribution div.
    • For a more responsive solution, I would change the max-width attribute on the .card__container element to 900px and get rid of the width, height and max-height attributes altogether. Especially when we have loads of components on the screen we don't want to be too prescriptive with heights and widths to give our designs as much flexibility as possible.
    • The breakpoint to the desktop media queries can probably come in at a smaller screen width
    • Probably put a tiny bit more horizontal padding on the container element.

    Looks fantastic though, keep it up!

    Fraser

    Marked as helpful

    1
  • Nick• 40

    @FrontEndNick2022

    Submitted

    Hi all, I was pretty pleased with how this one went with just two issues.

    1 - Can someone please take a look at how I resolved the background image? I found it a real pain tbh and not sure I've put it together in the best way possible.

    2 - This may be the stupidest question today but I can't for love nor money make the "Annual Plan" text bold without affecting the price at the same time. I gave it a custom class and set the font-weight to 700 etc, but that didn't work. I'm guessing it is something to do with inheritance perhaps? I tried all manor of declearing the parent classes in my CSS but still couldn't get it to change.

    Any feedback is much appriciated.

    Order Summary Component | HTML CSS Flexbox

    1

    Fraser Watt• 1,790

    @fraserwat

    Posted

    Hey nick, this is looking good!

    On your 2nd point, you should be able to solve this by putting font-weight: 700 in your .text-dark rule, also means you can get rid of .font-weight-dark class on the <p> element.

    Background image looks fine, although I would use max-width instead of width on .card. Less of an issue on a single component challenge like this one, but when you've got loads of components on screen you want them to be as responsive to screen size changes as possible so don't want to be too prescriptive on widths and heights.

    Keep it up! Fraser

    Marked as helpful

    1
  • namlh• 630

    @namlh023

    Submitted

    I've got an HTML issue that says "Element label not allowed as child of element div in this context. (Suppressing further errors from this subtree.)". Any ideas on how to fix this? Thanks a lot.

    Tip Calcuator React

    #react

    3

    Fraser Watt• 1,790

    @fraserwat

    Posted

    Hey, this is looking good! Couple of things i'd change:

    • I'm not quite sure the calculations are correct on the tip %, I enter $50 bill between 2 people at 50% tip and it says tip is $0.25
    • Your html validation issue might just be a typo <lable> as opposed to <label>
    • Would also quickly include your logo in <h1> tags as a screen reader would expect a header somewhere on page (you've got alt text in there so it still works)

    Great stuff though!

    Marked as helpful

    0
  • Fraser Watt• 1,790

    @fraserwat

    Posted

    This is looking really nice!! Only things I'd change:

    • Add a bit of border-radius to the image
    • Simplifying the HTML a bit - if you've got the <main> as a container, does it need the <div class="container">?
    • Using semantic HTML. The .attribution component can be a <footer>, what do you reckon the .card component should be?

    Keep it up! Fraser

    Marked as helpful

    0
  • Fraser Watt• 1,790

    @fraserwat

    Posted

    This is so nice! Might have to go back and redo my version of this... 👀

    0
  • Karabo Sambo• 240

    @blaqbox-prime

    Submitted

    I feel like it doesn't look quite right.🤔 I don't know if its the sizing or the scaling out of the site. Just your opinion on the final product would be appreciated greatly. and where else I could place my efforts in improving my markup and styling skills

    SunnySide Agency Landing Page

    1

    Fraser Watt• 1,790

    @fraserwat

    Posted

    I think it looks great! I find working with images really hard so you've aced this. I know the bit you're talking about, and maybe it could be avoided by having the breakpoint between the mobile and desktop views a bit earlier.

    Would also take a bit of time to look at the HTML issues the frontend mentor platform raised. It's mostly stuff around titles - change all but the first <h1> elements to <h2> elements, this will probably allow you to simplify your CSS as well!

    Keep it up! Fraser

    0
  • Fraser Watt• 1,790

    @fraserwat

    Posted

    This looks really great - very clean, especially at desktop width!

    Couple changes:

    • at mobile widths, you'll want to remove the width: 24rem on .card__image, as if you move the screen to 600-750 ish px wide the picture doesn't stretch across the whole div
    • Likewise, on your (max-width: 774px) media query, get rid of the height: on .card__body - you tend not to want to explicitly define the height in divs with content in them, and it's leading to a lot of white space around this width. This will introduce a padding issue, which you can also solve by removing the padding-bottom: 0; line from the same element.

    Keep it up!

    Marked as helpful

    1
  • Fraser Watt• 1,790

    @fraserwat

    Posted

    This works really nicely!!

    Couple of things to change:

    • <span> tags need to go inside a text element, whether that's a <p> tag or a <h1/2/3/etc>
    • There's a few things you need to fix in your HTML in the above report, e.g. lang attribute in the <head>.
    • Thinking about how this would be used in a real-life setting, you should probably set it so that the user can only upvote or downvote a comment once
    • If you have the upvote / downvote / reply elements as <button> elements, then people with accessibility needs will be able to tab to them (and it makes more sense semantically)

    But it works and looks great, keep it up!!

    Marked as helpful

    0