@BillyJoeDev
Submitted
Would love to get feedback on how I can improve on my CSS and HTML layouts.
Looking to hire developers?
@heyitsgany
@BillyJoeDev
Submitted
Would love to get feedback on how I can improve on my CSS and HTML layouts.
@heyitsgany
Posted
You've managed to recreate the design pretty faithfully, and your responsiveness works. Nice job!
However, there are a few things you need to look at:
mainContainer
class into a <main> element instead of the <div>).flexBoxContainer
class has a set of CSS properties that don't do anything to the style until you add a display:flex
to the class in a media query. I'd suggest grouping this all together in the media query.That's just a few of the things I could see while looking. Hopefully these give you some things to work on and help you learn in the process. You've done a great job, and you should be pleased with what you produced! Keep it up!
Marked as helpful
@Sloth247
Submitted
Hi, thanks for visiting this solution page. This is my first time building with React other than code along at tutorials. This project is probably not good to practice React, but I wanted to try from newbie challenges. I hope the methods are correct. This app should fully accessible and focusable. I am looking forward to hearing any feedbacks or comments especially from someone who is familiar with React.
@heyitsgany
Posted
This looks good, you've gotten pretty close to the design, nice work!
A couple of things to look at:
Also, the solution report is stating that you need to include a h1 element, although looking at your markup I can see it's there. I'm not sure if making it aria-hidden has caused this issue?
Marked as helpful
@drewhosick
Submitted
Would love to know if my React Components look sound and I used React in the proper way passing props etc...
@heyitsgany
Posted
I think it switching to the column layout below 1440px is an odd choice. You want to find the resolution where the horizontal layout starts looking cramped or broken and then switch to the vertical layout.
You'll want to think about adding a h1 tag somewhere, as semantically a page must contain a level one heading. As the design doesn't have a good place for it, I'd suggest adding a h1 tag with screen reader only styling (so it's only visible to screen readers), using the title of the challenge.
Also, if you want to get your positioning closer to the design I'd suggest making sure your min-height on your body is 100vh, then giving it a display: grid and place-items: center.
Otherwise, this is great; the styling on the card pretty much matches the design. Nice work!
@PabloMClementeP
Submitted
I appreciate your feedback
@heyitsgany
Posted
Looks good. Well done on getting it close to the design.
<div class="order-card">
to <main class="order-card">
.*,
*::after,
*::before {
margin: 0;
padding: 0;
box-sizing: border-box;
}
Marked as helpful
@airpoland
Submitted
Any feedback with regard to the noob / rookie mistakes are much appreciated.
@heyitsgany
Posted
Hey, nice job. You've got nice and close to the design and it looks good!
There are a couple of things that I've noticed that you could work on to improve it.
Marked as helpful
@ibraheemabdlhafeez
Submitted
Please check it and tell problems i made
@heyitsgany
Posted
As you've already mentioned, this website is not responsive. You'll need to use media queries to set the styling for mobile devices.
These are a few things to look into fixing with your design (although certainly not an exhaustive list).
Keep working on it, and you'll get there!
@Vinit-Kumbhare
Submitted
@heyitsgany
Posted
Just from a quick look at the live site and the code, these are a few things I've noticed.
Marked as helpful
@AliceMenzie
Submitted
@heyitsgany
Posted
Looks good! You've followed the design well. Just a couple of things to work on. Firstly, you haven't implemented the hover effects to the buttons and anchor tag.
Secondly, to help with accessibility (for screen readers) change the container div element to a main element instead.
Also, you haven't implemented the responsive design for mobile. So, your design touches the sides of the viewport at resolutions lower than 375px.
As for your card moving around on your background. I'm not sure what you mean.
@Jon-tim
Submitted
@heyitsgany
Posted
Nicely done! Your solution looks good, although looking at the live version on GitHub pages there are a few things that could be changed to improve it.
Firstly, changing the 'container' div to a main element would solve the accessibility issue that your solution is getting.
Secondly, your background image is mispositioned and doesn't repeat. So, look at making it repeat on the x-axis and let it position itself.
Otherwise nice work!