@DrougnovSubmitted almost 3 years ago
Would love any kind of feedback. Especially if there's any better way to do this challenge. Thanks in advance :)
Omar Mohy
@omarmohy98All comments
- @omarmohy98Posted almost 3 years ago
Hi@@Drougnov, wonderfully done! The code is remarkably responsive. Below is my feedback on potential improvements!
1.) your <a> tags not have discernible text that will make an accessibility issue, you can use aria-label="" to solve this problem, for example <a href="#"><i class="fab fa-facebook-f"></i></a> will be <a href="#" aria-label="Facebook Account"><i class="fab fa-facebook-f"></i></a>.
If you found my feedback helpful, please mark this comment as helpful. Wonderfully done. Happy coding!
Marked as helpful1 - @kmeganizSubmitted almost 3 years ago
Hi this is my first time doing challenge from front end mentor. I was feeling nervous and anxiety at beginning. I am trying to use as many as possible bootstrap library, as to not invent the new wheel. Before I saw other people solutions, mine struggle in making the card top part to be rounded, now realize, the card behind is different layer, so I have to apply the same code for the img.
I still feel this need improvement, I am open for your feedback.
Especially, I saw that between the image and the card there is a thin white line before the shadow there. Anyone can help me how to remove that?
@omarmohy98Posted almost 3 years agoHi@kmeganiz, wonderfully done! The code is remarkably responsive. Below is my feedback on potential improvements!
1.) let's put all elements in your solution into <main> element. Doing so will give your code more semantic meaning, will allow screen readers to know this is your main content and Fix the issues appear in Accessibility Report.
2.) All of your images should have alt tags with content within the quotations, regardless if whether or not they are purely decorative or not. You can put the aria-hidden="true"property in the <img> element to denote that the image is purely decorative (as is the case with your icons). Your music icon would look like <img src="images/icon-music.svg" class="img-fluid" alt="Music" aria-hidden="true"> .
3.) Careful with your headings. You should only use heading tags when the content is actually a heading. For example, I would replace your <h6> tags with <p> tags. <p> tags would make more sense here since that information is your content, not necessarily a heading and i would replace <h5 class="card-title text-center fw-bold pt-3 pb-2">Order Summary</h5> with <h1> tags that will help you to solve accessibility issues appear in your solution report.
4.) <div class="change col-4 text-end fw-bold my-4 me-4" href="#">Change</div>,here i see that you use href attribute that is not acceptable, i would replace that by the following <div class="change col-4 text-end fw-bold my-4 me-4"><a href="#">Change</a></div> and the same in your "cancel order" <div> tag.
If you found my feedback helpful, please mark this comment as helpful. Wonderfully done. Happy coding!
Marked as helpful0 - @jussi777Submitted almost 3 years ago
Feedback appreciated. Thanks!
@omarmohy98Posted almost 3 years agoHi@jussi777, wonderfully done! The code is remarkably responsive. Below is my feedback on potential improvements!
1.) Instead of <div class="profile"> , let's replace that with <main> element. Doing so will give your code more semantic meaning, will allow screen readers to know this is your main content and Fix the issues appear in Accessibility Report.
2.) All of your images should have alt attribute with content within the quotations, so in your <img src="images/image-victor.jpg" alt="" /> tag alt attribute may should have the value "victor picture".
3.) I recommend that you may have to replace your victor name <p> tag into <h1> tag, so your <p>Victor Crest <span>26</span></p> will be <h1>Victor Crest <span>26</span></h1>, that will help to solve the accessibility issues in your report.
4.) I recommend that you may have to replace your stats <p> tags into <h2> tags for example <p>80K</p> will be <h2>80K</h2>, that will help to solve the accessibility issues in your report.
If you found my feedback helpful, please mark this comment as helpful. Wonderfully done. Happy coding!
Marked as helpful0 - @eyagargahSubmitted almost 3 years ago
Hello, I need feedback on the CSS and the responsiveness of the page, please
@omarmohy98Posted almost 3 years agoHi@eyagargah, wonderfully done! The code is remarkably responsive. Below is my feedback on potential improvements!
1.) Instead of <div class="card"> , let's replace that with <main> element. Doing so will give your code more semantic meaning, will allow screen readers to know this is your main content and Fix the issues appear in Accessibility Report.
2.) All of your images should have alt attribute with content within the quotations.
If you found my feedback helpful, please mark this comment as helpful. Wonderfully done. Happy coding!
Marked as helpful0 - @sachdevavaibhavSubmitted almost 3 years ago
This is my first challenge on Front-End Mentor. Is my css code readable and maintainable. If no, then what are the things that I can improve. Your feedback is appreciated.
@omarmohy98Posted almost 3 years agoGood morning@sachdevavaibhav, wonderfully done! The code is remarkably responsive. Below is my feedback on potential improvements!
1.) Instead of <div class="main"> , let's replace that with <main> element and make it the top level in your code. Doing so will give your code more semantic meaning, will allow screen readers to know THIS is your main content and Fix the issues appear in Accessibility Report.
2.) Try to control images' width & height in style.css not in <img> element in your index.html.
If you found my feedback helpful, please mark this comment as helpful. Wonderfully done. Happy coding!
Marked as helpful1
