Skip to content
OverviewSolutions
18
Comments
8

Brendan Hyde

@hyde-brendan330 points

Hi! I'm currently learning front-end design in order to blend my passions for both art and computers. All my current Frontend Mentor solutions are being uploaded to https://github.com/hyde-brendan/hyde-brendan.github.io; please take a look and give me any feedback!

I’m currently learning...

JavaScript, CSS animation

Latest solutions

View more solutions

Latest comments

  • Brendan Hyde•330
    @hyde-brendanPosted about 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 about 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 about 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
  • Brendan Hyde•330
    @hyde-brendanPosted about 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 about 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 about 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
View more comments