Design comparison
Community feedback
- @AlexKMarshallPosted about 3 years ago
Visually this looks good but there are a few issues with the interactivity that you could update.
There are a few different images that you've attached onClick handlers to (the + and -, and the delete in the cart to name a few). These should all be button elements with onClick attached to the button.
As a user I should be able to close the shopping cart dialog by clicking outside of it, and as a keyboard user I should not be able to move focus outside of the dialog without closing it. The content behind the dialog should also be hidden from screen-readers while the dialog is open. I would suggest using the Dialog component from ReachUI as it solves all these accessibility issues, and you can style it however you choose.
I would also avoid using document.querySelector to add and remove classes. You should be able to do that in the JSX with conditional classNames based on your state variables. That will make the logic easier to extend and debug later.
Marked as helpful1@MilosSimic994Posted about 3 years ago@AlexKMarshall Thank you for reviewing my code, it's mean a lot to me!
I agree with you and I will replace all image tag with a button and attach onClick handlers on them.
Also, I understand that is not a good practice to selecting elements, but I have a problem selecting sibling elements, when I click on one of them. Perhaps I must change my logic for that.
Thanks again @AlexKMarshall
0@AlexKMarshallPosted about 3 years agoIf I understand correctly, you're having an issue with highlighting the selected image thumbnail below the main image on the page. And then are toggling on and off the active class for your .thumbnail class using css selectors.
An alternative would be to have
selectedImage
as a state variable. Referencing maybe the image name, or anything unique.Then where you return the image thumbnail you can have
<img className={`thumbnail ${img.name === selectedImage ? 'active' : ''}`}/>
And that will dynamically add the active class to the thumbnail, based on whether that image name matches the one that you've selected
Marked as helpful1@MilosSimic994Posted about 3 years ago@AlexKMarshall Thanks buddy, your feedback was helpful, i tried to correct certain parts of the code.
0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord