search

adobe / asset-share-commons

A modern, open-source asset share reference implementation built on Adobe Experience Manager (AEM)
https://opensource.adobe.com/asset-share-commons/
Apache License 2.0
88 stars 107 forks source link

Opening up cart modal appends new cart instance to the DOM #285

Closed m-suchorski closed 6 years ago

m-suchorski commented 6 years ago

Describe the bug When user opens a cart modal by clicking the cart icon it creates a new instance of the modal in the DOM instead of opening up the existing one.

Environment

To Reproduce Steps to reproduce the behavior:

  1. Go to any page that has the cart icon
  2. Click on the cart icon to open up the cart modal
  3. Cart is visible but it is a new instance of the modal that has been appended to the DOM. See screenshots below.

Expected behavior Cart modal should show up without it being appended to the DOM again (so there is only ONE cart instance on the page instead of n instances - n being the number you click on the cart).

Screenshots Opening the cart up for the first time: cart-first

Opening the cart up for the third time (note three forms instead of one): cart-3

davidjgonzalez commented 6 years ago

Labeling as enhancement since AFIAK this doesnt break anything. If it's actually breaking something, please update the ticket.

m-suchorski commented 6 years ago

Well it actually does break quite much - making a custom cart with it's event listeners can cause the page to be really laggy after opening up the cart for a few times. Please consider marking it as a bug, because I'm sure it is not a good thing to append stuff in DOM instead of just making them visible again.

godanny86 commented 6 years ago

@m-suchorski I think the label is really just semantics but I agree it can lead to performance issues. We can prioritize making this update, I'm assuming the LOE should be relatively small.

davidjgonzalez commented 6 years ago

I expect this change will need to be in ...

and should leverage SemanticUI modals' functions for self clean-up. I would've thought Semantic's Modal Lib wouldve handled this so it'll require investigation as to why its not.

m-suchorski commented 6 years ago

Thank you guys, I hope you can resolve that issue without any problems.

godanny86 commented 6 years ago

agreed @davidjgonzalez wrt to Semantic UI modal, should follow Semantic's best practices. Potentially related is that we perform a post to generate a new modal even if the cart contents has not changed. I think we should also consider some very basic state management to the cart to avoid additional ajax calls.

godanny86 commented 6 years ago

@m-suchorski see PR #286. Will you pull the latest from the develop branch and verify that the issue is fixed and your custom cart modal continues to work as expected?

m-suchorski commented 6 years ago

@godanny86 my issue has been fixed on your develop branch - thanks! I've been doing some testing around it and everything seems to be working just fine.