Responsive and functional E-commerce page

Community feedback

• 

  Jo89 😈•380

  @AhmadYousif89Posted over 1 year ago

  Hi Gabrial 👋

  Your solution looks beautiful 👍 but I just noticed the following issues with your app and maybe fixing these issues will make your app even more appealing to the users :

  • I noticed that the backdrop in the lightbox is not the highest element on the DOM in term of z-index (usually the backdrop on any modal should have the highest z-index) and therefor user can click and interact with element beneath it and of course this is not good behavior.

  • because of the first issue I can open the cart and then open the lightbox and see that the cart is actually visible and popping through the backdrop.

  • would be a good user experience if I can open the cart and close it with just clicking away (anywhere on the page)

  • try adding a line-through the original price indicating a discount has happened to it and that it now has a discounted price (this is just a minor visual change) but it adds to the whole user experience (IMO)

  • would be good if I can close the side menu by clicking on the backdrop (and again I see the same problem as mentioned in the first issue)

  you have a nice day 👍

  Marked as helpful

  1

  ---

  

  Gabriel Gusso•250

  @g-pgPosted over 1 year ago

  @AhmadYousif89 Thank you for your feedback!

  Fixed those issues. It led me to learn how to create React custom hooks so I could repeat the "click outside element" logic on the cart popup, the gallery modal and the burger menu. Learned a lot.

  Thanks again!

  1

  

  Jo89 😈•380

  @AhmadYousif89Posted over 1 year ago

  @g-pg

  I still can interact with the elements underneath the backdrop 😐

  1

  

  Gabriel Gusso•250

  @g-pgPosted over 1 year ago

  You're right! I only included logic to close the modal when the user clicks outside. Since this modal is not a warning or something similar, maybe it isn't a problem to let the user interact with the cart button, for instance, as long as the modal is not visible anymore.

  Appreciate your attention very much, @AhmadYousif89.

  0

  

  Jo89 😈•380

  @AhmadYousif89Posted over 1 year ago

  @g-pg

  I mean for such a dummy/training application yes it doesn't matter that much but imagine you delivering this work to a real customer or your boss at your company instructed you to build a real gallery with a production level lightbox that will serve actual users then it will be a big problem, it's not just a UI/UX issue but it's also bad in term of developer point of view, I mean like if I was to hire a dev for example I would be looking for the one that care about the code quality and also deliver good user experience and care about that little details (of course I'm not taking about you personally).

  0

  

  Gabriel Gusso•250

  @g-pgPosted over 1 year ago

  @AhmadYousif89

  To be honest, I usually see modals that behave the way mine does, ie, it closes when the user clicks outside of it. If my client or boss specifically asks for a modal that only closes when the close button is clicked, of course I will code it to behave that way. I don't think there is a correct and a wrong way to adress this. If there is, I sincerely want to know!

  0

  

  Jo89 😈•380

  @AhmadYousif89Posted over 1 year ago

  @g-pg

  I was talking about the backdrop issue (the z-index issue) not the closing behavior of it, of course this is the correct behavior or let's say the expected behavior for closing the backdrop

  0

  

  Gabriel Gusso•250

  @g-pgPosted over 1 year ago

  @AhmadYousif89 Sorry, I'm not sure if I follow.

  The modal is the highest z-index element, is it not? Before, you were right, the cart could show up in front of the modal. Now, when I open it, the cart popup automatically closes. The only thing I kept was the possibility to click on the cart button with the modal opened. But when I do the modal closes. I didn't completely disable outside clicks.

  You're saying the way I implemented these fixes is wrong or these fixes are not showing for you?

  0

  

  Jo89 😈•380

  @AhmadYousif89Posted over 1 year ago

  @g-pg

  Now closing the cart with clicking outside the cart modal is awesome, but that doesn't fix the main issue with the backdrop overlay when the lightbox is open or the side menu is open, again the issue is that I can interact with the underneath elements while the backdrop overlay is open and that is not a good experience nor correct behavior, because the backdrop overlay should be covering the entire screen and making the user focus and interact only with what's inside/above that overlay.

  I hope that made sense 🙂

  0

  

  Gabriel Gusso•250

  @g-pgPosted over 1 year ago

  @AhmadYousif89

  Fixed it!

  1

  

  Jo89 😈•380

  @AhmadYousif89Posted over 1 year ago

  @g-pg

  Looks awesome 👍

  1
• 

  S MD suleman•3,530

  @sulemaan7070Posted over 1 year ago

  hey 😄Gabriel Gusso, congratulations on completing the challenge... here are a few tips to make your site better.

  1.Firstly the logo it seems you have used regular text...you can find the logo here frontendmentor-ecommerce/src/assets/svg/logo.svg

  2.It seems you are using 100% of the width for the nav.. to make it design accurate you use 75% to 80% of the width and margin auto on the right and left..

  3.The button seems to be missing the box-shadow which would make the button cooler..

  Everything else is great!! happy coding💯😄👍🏻

  Marked as helpful

  1

  ---

  

  Gabriel Gusso•250

  @g-pgPosted over 1 year ago

  @sulemaan7070

  Hey, thanks for your feedback! I fixed those issues.

  1
• 

  Abdul Khaliq 🚀•72,560

  @0xabdulkhaliqPosted over 1 year ago

  Hello there 👋. Congratulations on successfully completing the challenge! 🎉

  • I have other recommendations regarding your code that I believe will be of great interest to you.

  HTML 🏷️:

  • This solution generates accessibility error reports due to non-semantic markup, which lack landmark for a webpage

  ---

  • So fix it by wrapping the whole content inside the semantic element <main> in your index.html file to improve accessibility and organization of your page.

  ---

  • What is meant by landmark ?, They used to define major sections of your page instead of relying on generic elements like <div> or <span>

  ---

  • They convey the structure of your page. For example, the <main> element should include all content directly related to the page's main idea, so there should only be one per page

  ---

  HEADINGS ⚠️:

  • And, This solution had also generated another accessibility error report due to skipping heading levels

  ---

  • We want to avoid skipping heading levels, make sure to start with <h1> and working your way down the heading levels (<h2>, <h3>, etc.) helps ensure that our document has a clear and consistent hierarchy. Read more 📚

  ---

  • Because skipping heading levels is a poor practice from the perspective of information design, whether we are talking about web pages, books, journal articles, or about anything else. You can not only confuse screen readers but all readers when you don't follow a consistent, logical pattern with your heading structure.

  ---

  I hope you find this helpful 😄 Above all, the solution you submitted is great !

  Happy coding!

  Marked as helpful

  1

  ---

  

  Gabriel Gusso•250

  @g-pgPosted over 1 year ago

  @0xAbdulKhalid

  Thanks for your feedback! Fixed almost all of them, but I'm still getting a warning to ensure "landmarks are unique". It points to the <nav> element.

  I used a <nav> tag to wrap the main nav and another <nav> to wrap the cart and profile buttons, since they serve distinct purposes. Is it wrong?

  Thanks!

  0