Opening up cart modal appends new cart instance to the DOM

[m-suchorski]

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

• AEM Version: 6.4
• Asset Share Commons Version: 1.6.6
• Author, Publish or both? Publish

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:

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

[davidjgonzalez]

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

[m-suchorski]

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]

@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]

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

• https://github.com/Adobe-Marketing-Cloud/asset-share-commons/blob/develop/ui.apps/src/main/content/jcr_root/apps/asset-share-commons/clientlibs/clientlib-site/js/semantic-ui/modal.js

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]

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

[godanny86]

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]

@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]

@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.