fix: modal using animation frame and tests

[suwarnoong]

Tests for Modal #201

[xujing-shen]

why do we need to set this prop to false, will it set to false after clicking the button

[suwarnoong]

yes, that is because the click event only emit "close" and "update:active" and it does not set the active to false.

[xujing-shen]

i think we should add "this.isActive = false" in the private close()

[ChristianKienle]

I think adding this.isActive = false will fix the issues @suwarnoong has down below (the sync-issue).

If your test looks exactly like the example then the sync issues will vanish.

This is because the example is going though the vue template compiler which is causing to catch the update:active-event and updates the state.

Our modal breaks down once the consumer is doing this:

<FdModal :active="false">…</FdModal>

and not

<FdModal :active.sync="active">…</FdModal>

Meaning: Our modal relies upon the fact that the consuming app is either using .sync or listening for the update-event accordingly + is binding the active-prop.

For this PR I suggest to do what @xujing-shen suggests but make a mental note (=> or an issue) to have a closer look at the modal component again. I think FdModal should me more resilient and support different modes of operation...

[ChristianKienle]

Note to myself: This is maybe a great question for our Q/A with Chris.

[suwarnoong]

Adding this.isActive = false in the close() will only close the FdModal for the very first time. But isActive value (in Modal.vue) is not propagated to parent (Example.vue). Meaning, the parent variable will still be true. Subsequent, the showModal will not trigger the watch for the active.

If the consumer doesn't use .sync, alternatively, they could listen on @close event and set the parent variable to false.

But, in my personal opinion, perhaps, we should only let parent to hide/show through hide() show() methods instead of just a variable. But this implementation will be different with fundamental-react. Are we allow to have different implementation?

<Modal onClose={this.showHideModal} show={this.state.bShowInfoModal}>
  ...
</Modal>

// show / hide information modal
showHideModal = () => {
  this.setState(prevState => ({
    bShowInfoModal: !prevState.bShowInfoModal
  }));
};

[ChristianKienle]

You are correct. If the consume is not using :visible.sync then it won't work. This is what I meant by saying:

Meaning: Our modal relies upon the fact that the consuming app is either using .sync or listening for the update-event accordingly + is binding the active-prop.

In the end, :active.sync="active" is syntactic sugar for:

<FdModal @update:active="active = $event" :active="active" … >

I think, when I wrote the code for FdModel I was heavily influenced by: https://element.eleme.io/#/en-US/component/dialog.

We are allowed to have different implementations. After all: We are all using different frameworks which have different features and philosophies. :)

With regards to your "methods-idea": Personally I think calling a method is less convenient than to have a sync-prop. It is also not less prone to errors. But: I think there is no right and wrong and there is no best way to do things. Every solution has pro and cons. I also think that bootstrap-vue has a different but very well designed solution:

https://bootstrap-vue.js.org/docs/components/modal#toggle-modal-visibility

Their solution is more or less what I had in mind for FdPopper. :D The world is so small.

If you want to add an open(…) method: Why not? :)

[ChristianKienle]

I just tested our modal component and it does not seem to work. I mean the default example in our docs... Does it work for you? :D

[suwarnoong]

Oops. doesn't seems to work too. The culprit is the .sync after :active. Once I remove that, it is fine.

But the X button on modal header isn't closing. If we add this.isActive = false in the close() as suggested by xujing, we also need to add ignoreElements on the ClickAwayContainer.

[ChristianKienle]

Oops. doesn't seems to work too. The culprit is the .sync after :active. Once I remove that, it is fine.

But the X button on modal header isn't closing. If we add this.isActive = false in the close() as suggested by xujing, we also need to add ignoreElements on the ClickAwayContainer.

#230 should solve this for you. You could just merge this PR into your branch...

However, I am wondering how and why this worked in the past - also without the fix.

[suwarnoong]

The modal works now in docs.
But the test-utils doesn't seems working prop.sync, even after I set the sync: true during mounting. In this case, I am still updating the active prop manually.

@ChristianKienle Should this PR be kept as multiple commits since there is a merge from other branch? Or perhaps combine as single commit.

[suwarnoong]

This should work for prop.sync (vuejs/vue-test-utils#1062) but no luck

[ChristianKienle]

The modal works now in docs.
But the test-utils doesn't seems working prop.sync, even after I set the sync: true during mounting. In this case, I am still updating the active prop manually.

@ChristianKienle Should this PR be kept as multiple commits since there is a merge from other branch? Or perhaps combine as single commit.

I think you can squash the commits and turn multiple commits into a single one. You have the option to select that option by clicking on the arrow of the merge button.

[suwarnoong]

the update:active is not updating active to false. Hence, perform manual update for this test.

[suwarnoong]

Seems like this PR is no longer relevant. Closed for now