Hello! I have completed this for a while now, however I still wanted to show it here. Your comments are highly welcome.
LiBee
@Li-BeeAll comments
- @OlamstixSubmitted over 2 years ago@Li-BeePosted over 2 years ago
Well done on the design also looks good on the mobile version π. I have a few comments for you consideration which I have listed below:
-
Remove the
width
that you have applied to the star icons they automatically scale at the moment the icons look stretched -
You need to add the background images to meet the design I could not see them - but it looks good as is too
-
To clear the accessibility point change the
<section class=
reviews>
to a<div>
element instead. You require at least one heading within<section>
Marked as helpful0 -
- @WangszzSubmitted over 2 years ago
-
yes, I found some hard parts while building this project. that problem appears because i was a newbie in SAAS and I try to use SAAS in this project.
-
I feel confident at all.
-
no, i don't have
@Li-BeePosted over 2 years agoGood job! To fix the accessibilty issues you need to add a landmark. If you add
<main></main>
around you code this should solve the issue for you. Link to more information if you want it. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/main0 -
- @DefinitelyObsessedSubmitted over 2 years ago
Feedback welcome .
@Li-BeePosted over 2 years agoGood job on the challenge - well done! π A have a few comments for your consideration - i have listed below for ease :
- The card is not quite in the center consider using flexbox to center the card component:
body { display: flex; justify-content: center; align-items: center; flex-direction: column; }
-
The titles Sedans, Suvs and Luxury need to capitalised in the design so:
text-transform: uppercase;
-
Compared to the design you need to smooth corners of the card component do add a border radius
-
Compared to the design the background is not white (#fff) need to add shade to the background.
-
To fix your accessibility issues add
<main></main>
around your code.
Marked as helpful1 - @riverCodesSubmitted over 2 years ago
Hi, please give me feedback on the responsiveness of this design!
The desktop design looks close to the original, but I am confused about how to achieve the mobile design. Should I use media queries? How do I make the desktop design transition into the mobile design?
I am confused with responsive design in general, so any advice on how to alter my code to make it more responsive would be super amazing!
@Li-BeePosted over 2 years agoHi similar to what @Kamasah-Dickson said it is better to start from mobile first as when you expand from mobile to desktop the adjustments you need to make is less in comparison to doing them the other way round.
What I did on this challenge was to start with mobile first and I put the card in an article wrapper
<div>
and did a<width>
of 87.2% and<margin>
of 0 and auto. So when the viewport is for example 375px the card width is 327px and there is space between the card and end of viewport.Link to my challenge: https://www.frontendmentor.io/solutions/order-summary-component-using-mobile-first-approach-and-css-grid-c-HmAvKIr
I am not saying it is perfect as I am still and learner and still learning about mobile first design but it may help π
Marked as helpful1 - @MilleusSubmitted over 2 years ago
This one's tricky with the image centering but I think I've found a nice way to always have the image centered even with larger heights or font sizes using CSS grid.
I'd appreciate it if I could get feedback on how to improve, especially on the use of semantic HTML and accessibility considerations. Thanks everyone o/
@Li-BeePosted over 2 years agoWell done on the design. I think you need to add sr-only class to you
<h1>
element. As the title Profile Card Component is showing on the screen.1 - @Li-BeeSubmitted over 2 years ago
Hi everyone π - please see my solution to social proof section. Let me know your thoughts and if anything is not in line with best practice. Note my use of flexbox - would it have been better to use CSS grid - in my view I thought this was not required as doing a 1D design.
@Li-BeePosted over 2 years agoAlso I did max-width on the testimonials at desktop view - is this the correct thing to do?
0 - @nakoyawilsonSubmitted over 2 years ago@Li-BeePosted over 2 years ago
This is really good π. If I may ask where did you learn HTML CSS - did you use a course?
1 - @KimJunpyoSubmitted over 2 years ago
Feedback welcome.
-
It was my first time working on a project, so it was very difficult. However, I think I learned a lot by searching for tags and attributes.
-
I'm not sure if the code configuration was efficient. Also, I'm not sure because I've never seen anyone else's code.
-
I would really appreciate it if you could give me feedback on the best practices and the different parts of my code.
@Li-BeePosted over 2 years agoHey looks really good well done π
I think you need to change your font on the desciption text - currently different to the design.
Also to clear your accessibility issues add
<main></main>
around you code as such, to give a landmark (here is more info on it if you are [interested] (https://dequeuniversity.com/rules/axe/4.3/landmark-one-main?application=axeAPI).<body> <main> <div class = "grid">...</div> <div class="attribution">...</div> </main> </body>
0 -
- @Sloth247Submitted about 3 years ago
Hi there, thanks for visiting my solution. I added animation to main hero image and card and make all interactive elements accessible and focusable, including svg icons.
I still have to improve the following items;
- The overlap of phone image and the card is not beautiful. I don't want to change the size of phone image by display size so much.
- The animation is working but they start from
translateY(0)
position and move to the designated position at last. I don't want to let it happen but I could not figure out.
Any other feedbacks are welcome.
@Li-BeePosted over 2 years agoHey this looks great. I am also trying to finish this at the moment. I do have a quick question is thats okay? I am new to website design and have been having problems with the background images.May I ask the following questions:
- Why for the mobile image did you put the background image in the header and not in the
<body>
?
.header { padding-top: 2.5rem; background-image: url("../../img/bg-main-mobile.png"); background-repeat: no-repeat; background-position: bottom left; }
- For the tablet version and desktop version I noted you then put the background-image in the
<main>
may i ask why you did this?Also why did you increase thebackground-size
to 150% for tablet and 130% for desktop? How did you come up with these measurements?
@media screen and (min-width: 40em) { body { background-image: url("../../img/bg-main-tablet.png"); background-repeat: no-repeat; background-position: top -10rem left -20rem; background-size: 150%; } } @media screen and (min-width: 62.5em) { body { background-image: url("../../img/bg-main-desktop.png"); background-repeat: no-repeat; background-position: top -15rem left -18rem; background-size: 130%; }
0 - @lnaranjocSubmitted over 2 years ago
Hello Everyone! This is my very first challenge in Front End Mentor. You may laugh, but I worked hard to get this QR code done.
I would really appreciate any feedback. I am sure there's things I can improve, or make in a different (maybe more efficient) ways.
Thank you very much in advance!
@Li-BeePosted over 2 years agoWell done - close to the design. To fix the accessibility issues you need to add a <main></main> landmark
<main> <div class="container"> .... </div> </main>
0 - @therifianSubmitted over 2 years ago@Li-BeePosted over 2 years ago
Very close to the design. To fix the accessibility issues you need to add a
<main></main>
landmark<main> <div class="container"> .... </div> </main>
2 - @AkadzaniSubmitted over 2 years ago
I would like to get the feedback for how i write the code for this challenge.
@Li-BeePosted over 2 years agoPretty close to the design - well done π. Compared to the design i think you need to make the heading smaller and add a bit of space between the bottom of the paragraph and the bottom of the card.
To fix your accessibility issues you need to add
<main></main>
to your html - to add a landmark.<body> <main> <div class="qr-code container"> .... </div> <div class="attribution"> </div> </main> </body>
Marked as helpful0