Skip to content
OverviewSolutions
18
Comments
8
Brendan Hyde
@hyde-brendan

All comments

  • Brendan Hyde•330
    @hyde-brendanPosted almost 3 years ago

    Hi! Great job completing this challenge! Some comments:

    • Using a % value on border-radius tends to be uneven on elements that aren't square; you can see on your <main> how the curve is stronger on the vertical side than the horizontal. Best to stick to an explicit px or em value for this case.
    • Instead of setting the vertical margin to 10%, you can center items vertically by placing the below CSS under your <body>:
    min-height: 100vh;
display: grid;
place-items: center;

    (Note you'll have to set position: absolute; on your .attribution element as well so the above works.)

    • To fix the Page should contain a level-one heading issue, change your #title element from <p> to <h1> (and adjust accordingly).
    • For next time, try playing around with box-shadow to get the slight shadow on the card!
    Marked as helpful
    2
  • Brendan Hyde•330
    @hyde-brendanPosted almost 3 years ago

    Hey there! Great job with this solution. You can center the component vertically by:

    • On <body>, add:
    min-height: 100vh;
display: grid;
place-items: center;
    • Remove or set display: none; to the #99fd803e-8fff-4139-9542-d0a0011cc7ff-quickmenu <div> after the hero <div>.

    Also, to fix the Document should have one main landmark issue, change your hero from a <div> to a <main>.

    Marked as helpful
    1
  • Brendan Hyde•330
    @hyde-brendanPosted almost 3 years ago

    Hi! Nice job completing the challenge. A couple comments:

    • The images becoming squashed whenever the name overflows to a second line can be fixed by adding an align-self to either start or center to your <img> elements. On that note though, the images are a bit larger than on the design (around 30px); you can afford to shrink them.
    • On that note, I would reduce the font size of the <h1> elements to prevent the peoples' names from taking multiple lines. The design has the same size as the main text (13px).
    • I would increase the font size of the .main-text elements to somewhere between 18px to 20px.
    • Remove the background-size property from .order-1; the stretching doesn't make the quotation mark look nice, and once you shrink the card header a bit it'll look more like the design anyhow.

    Hope this helps! For future work, see if you can use grid-template-areas for the layout, and multiple media queries for multiple layouts for tablets.

    Marked as helpful
    0
  • bikeinman•1,080
    @BikeInManSubmitted almost 3 years ago

    Can you please check if this meets all the requirements ? Also, suggestions for improvement, alternate ideas are welcome and very much appreciated. Thanks in advance.

    HTML5 | CSS | Grid | JavaScript

    2
    Brendan Hyde•330
    @hyde-brendanPosted almost 3 years ago

    Minor oversight, but the error message for the Last Name field says "First Name cannot be empty".

    0
  • Brendan Hyde•330
    @hyde-brendanPosted almost 3 years ago

    Hi, great work! The validation handling is a little buggy atm; the error message only shows up on the First Name field until it's valid, then only shows on the Last Name field, and so on. Instead, it should display the error message on every field that isn't valid.

    I would change your implementation from a if-else chain to a "click" event listener; upon pressing the submit button, the function should access each input field, validate its value, and show/hide the corresponding error message if invalid. You can check my solution for this challenge out for reference.

    Besides that, some other minor visual issues:

    • An invalid field should also add a red outline around the input box, and display an error icon on the right.
    • The input fields and submit button are missing a little border-radius that the design has.
    • The vertical padding for the form is a bit tight, I would increase it a bit.
    • During the middle-size layout, the "free 7 days" card is able to have a different size than the form card, while on the other two layouts it remains the same as the form card.

    Hope this helps!

    Marked as helpful
    0
  • Brendan Hyde•330
    @hyde-brendanPosted almost 3 years ago

    Hey, great start! You handled the most notable part of the design (the offset cards) well, but there's a few immediately noticeable issues I would get on:

    • There's no mobile layout; if you reduce the screen size the reviews' cards will overflow off the screen. The general idea for these sorts of things is to design the page for mobile layouts first, then add media queries to change certain CSS properties when the screen size is larger than a given value.
    • The background patterns provided in the images folder is missing from your design, as are the background color over the 5-star ratings cards.
    • To align the page's content to the middle of the screen one common method is to include the following to your main body element:
    display: grid;
place-items: center;
min-height: 100vh;
    • For the Document should have one main landmark and All page content must be contained by landmarks warnings, there's a couple of HTML elements that are treated as the "main landmark", including header, nav, main, and footer. For this particular page I would just wrap all your content in a <main> element to resolve that issue.
    • <article> elements should use a heading element to identify the content for it. You can resolve this on .reviewed by making the names of the reviewers a <h2>.

    Hope this helps!

    0
  • Fai•140
    @FsaneaSubmitted almost 3 years ago

    Please check my code & design and provide feedback. I have struggled in defining the font wight & color. I think I need to structure the code more better.

    Built with CSS grid, flexbox.

    1
    Brendan Hyde•330
    @hyde-brendanPosted almost 3 years ago

    Hey, great job with this! A few comments that should hopefully help you out:

    • For the Document should have one main landmark and All page content must be contained by landmarks warnings, there's a couple of HTML elements that are treated as the "main landmark", including header, nav, main, and footer. For this particular page I would just wrap all your content in a <main> element to resolve that issue.
    • The Page must contain a level-one heading warning can also be easily resolved by changing your <h2> into a <h1>.
    • The Images must have alternate text error is for accessibility reasons with people viewing the page with a screen reader. Since the card images are mainly decorative, you can just add an empty alt attribute (i.e. alt="") to the <img> elements.
    • Including the header in the grid is interesting, but I would remove the top-most row and set the header padding to adjust, currently there's a large amount of whitespace at the top.
    • It's not required, but clamp() is a very neat function that can be used with font-size to have more dynamic font size changes based on the screen size. You can use it to make the same header increase in size when you're on the desktop view as opposed to the mobile view.

    Hope this helps!

    Marked as helpful
    1
  • Brendan Hyde•330
    @hyde-brendanPosted almost 3 years ago

    Great work! Two minor comments from me:

    • The upvote/downvotes are a little wonky:
      • There's no visual indication whether you pressed + or –, and when you press one, the other only cancels out the upvote/downvote; you cannot convert an upvote into a downvote without refreshing the page.
      • Having it so if you reclick the + or – undoes the upvote/downvote, and directly clicking the other jumps over the initial value (i.e. 11 -> 13 on amyrobson's comment) would make it more intuitive.
    • The design has icons for replying, editing, and deleting comments your solution currently does not have.
    Marked as helpful
    0