@Irving2797
Submitted
Any feedback is welcomed and appreciated! :)
Looking to hire developers?
@hyde-brendan
@Irving2797
Submitted
Any feedback is welcomed and appreciated! :)
@hyde-brendan
Posted
Hi! Great job completing this challenge! Some comments:
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.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.)
#title
element from <p>
to <h1>
(and adjust accordingly).box-shadow
to get the slight shadow on the card!Marked as helpful
@lpdm1
Submitted
First ever challenge. I'm studying Front-End for about 3 weeks now. Any suggestions?
@hyde-brendan
Posted
Hey there! Great job with this solution. You can center the component vertically by:
<body>
, add:min-height: 100vh;
display: grid;
place-items: center;
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
@francobwogo
Submitted
Feedback needed.
Thanks, Francis.
@hyde-brendan
Posted
Hi! Nice job completing the challenge. A couple comments:
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.<h1>
elements to prevent the peoples' names from taking multiple lines. The design has the same size as the main text (13px)..main-text
elements to somewhere between 18px to 20px.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
@BikeInMan
Submitted
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.
@hyde-brendan
Posted
Minor oversight, but the error message for the Last Name field says "First Name cannot be empty".
@arunsingh009
Submitted
Feedback on my code will be appreciated. Thankyou🤗
@hyde-brendan
Posted
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:
border-radius
that the design has.Hope this helps!
Marked as helpful
@Sundaram218
Submitted
Any Suggestions? Thanks!
@hyde-brendan
Posted
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:
body
element:display: grid;
place-items: center;
min-height: 100vh;
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!
@Fsanea
Submitted
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.
@hyde-brendan
Posted
Hey, great job with this! A few comments that should hopefully help you out:
header
, nav
, main
, and footer
. For this particular page I would just wrap all your content in a <main>
element to resolve that issue.<h2>
into a <h1>
.alt=""
) to the <img>
elements.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
@sakshamian
Submitted
Hello everyone, this is my submission for the challenge. Hope you like it. Any suggestions and advices are most welcome.
@hyde-brendan
Posted
Great work! Two minor comments from me:
Marked as helpful