Tradeshift / tradeshift-ui

The Tradeshift UI Library & Framework
https://ui.tradeshift.com
Other
33 stars 45 forks source link

Modal is not closing completely, the grey cover is still visible #913

Closed MirelaPaun closed 4 years ago

MirelaPaun commented 4 years ago

Describe the bug Modal is not closing completely, the grey cover is still visible

To reproduce Open a Modal and close it, the grey cover will remain visible and in the DOM it remains

<div class="ts-modal-cover ts-visible"></div>

, which blocks the user to do any other action in the page.

This issue is reproducing after we bump the version of ts-ui-comp from 12.2.10 to 12.3.6. From our investigations this commit might broke this: https://github.com/Tradeshift/tradeshift-ui/commit/49593c08ef5853f18855bf4dff3cc1e5016b9f85

Expected behavior The Modal should be closed completely.

Framework usage React 16.8.6 TS UI Comp 12.3.6

Desktop (please complete the following information): Chrome

Smartphone (please complete the following information):

Additional context image

Kian-Esmaeilpour commented 4 years ago

It's not reproducible in the ui.tradeshift.com, So I had to go through your code to see what you are doing. You are using react-tradeshift-ui, and in their implementation they are considering that data-ts.open is working for both opening and closing the modal which is not a part of the documentation.

To open the Modal, you can either set the data-ts.open attribute to true (using jQuery or something like that) or you can use the following API.

So you can create an issue on that repo for it 👍

In future please provide more information about how you are using the ui-component 🙏

MirelaPaun commented 4 years ago

@esmailpour-hosein Sorry if it wasn't clear my description, I always try to give as many info as I know, that's why I was looking in your code to see which commit might caused this issue. It it not my intention that someone else waste his time. :(

And sorry for asking again, but how can I fix this on our side because data-ts.open attribute seems to work fine, true when the Modal is open and false when it's closed? Or was I understanding wrong your suggestion? image

image

kiwdahc commented 4 years ago

@esmailpour-hosein Can I ask how the modal and the background of the modal being able to be not in the same opened state not a bug within the TSUI codebase? Our modals also broke without changing any code and while only updating tsui to a new minor version.

Kian-Esmaeilpour commented 4 years ago

@MirelaPaun No problem :) I just wanted to provide a helpful feedback on the description. It can be easier to find the related code for us, so don't go through the pain to find the code that much, the part that is hard to understand for us is the reproduction steps. To help to find the issue faster we would really appreciate it if you provide your code or the way that you are trying to use it. We have template watch and also API so it can be tricky to find which one is broken.

Open a Modal and close it This is vague, if all the modal in the documentation was broken this was good enough but when all the examples in documentation is working then we need to know a bit more about how you are trying to use them, then we can try it the way that you are using them.

You are always more than welcome to ask question. :) I also tested it with the data-ts.open and it seems working fine with it as well, even though there's no documentation that says this can be used for closing the modal as well AFAIK. So this changes what I was thinking is the reason for the issue! Still I can't reproduce it.

Kian-Esmaeilpour commented 4 years ago

@esmailpour-hosein Can I ask how the modal and the background of the modal being able to be not in the same opened state not a bug within the TSUI codebase? Our modals also broke without changing any code and while only updating tsui to a new minor version.

@kiwdahc Can you provide more information on how to reproduce the issue? All the examples in the documentation working fine, and also tried to check it with the attribute change and it's still working fine.

Kian-Esmaeilpour commented 4 years ago

For future reader who maybe have the same issue:

In the previous version all of the elements related to the modal was only inside the modal element in the DOM, so by putting a condition and not rendering the modal at all it could feels like that you are closing the modal, but actually it just removes the modal template from DOM, and not completing the closing cycle of the modal, so the cover which is not in the same node won't be removed. The following example won't work :

       // Won't work
    renderModal() {
        const { show } = this.state.modal;
        return (
            show && (
                <Modal title="some title" isOpen={show}>
                                   some content
                </Modal>
            )
        );
    }

You need to remove the condition before the modal and close the modal with only changing the isOpen value.

       // Works
    renderModal() {
        const { show } = this.state.modal;
        return (
            <Modal title="some title" isOpen={show}>
                             some content
            </Modal>
        );
    }

Thanks to @MirelaPaun for helping in the debugging process. 🙏