Skip to content

Conversation

@NoelDeMartin
Copy link
Contributor

I have been working with the library and it isn't clean having to add the modals-container all the time. With this option, it can be injected into the document once the plugin is loaded.

@euvl
Copy link
Owner

euvl commented Jun 14, 2018

It's arguable, i will approve the pr but the idea of modern frameworks is to make it less of black box and more declarative.

This feature sells declarative way of doing things for black-boxy functionality. For example if something does not work, only you, me and God will know what is going wrong with lib. it is slightly vague example (it was only one line), but if you scale the example, you can see what i mean.

For example, in react you will never do this kind of things and it is for a good, well-thought-through reason.

@euvl euvl merged commit 6948bc0 into euvl:master Jun 14, 2018
@NoelDeMartin
Copy link
Contributor Author

Yeah I understand what you mean, there is a blurry line between "magic" and acceptable automation. Magic is not usually a good thing because it worsens debugging and understanding. In this case I think it's acceptable because interacting with the plugin should only be done when actually opening modals, and the bootstraping should be encapsulated to the init function, like any other vue plugin. What I am trying to achieve is calling Vue.use(VueModal); and be done with the plugin registration, like any other plugin. But I understand it would also break functionality, that's why I made it as an option and the default behaviour is still the same as before.

If you think about it, injecting this is similar to what is already being done of injecting CSS styles.

@dgroh
Copy link

dgroh commented Jun 30, 2018

It's already documented but not implemented? Per documentation it should be possible to add the modal container to the body by using:

Vue.use(VModal, { dynamic: true, injectModalsContainer: true });

I get the following warning tho:

[vue-js-modal] In order to render dynamic modals, a component must be present on the page

@NoelDeMartin
Copy link
Contributor Author

@dgroh it should be working, but it was added quite recently, so make sure to have version 1.3.16 or higher. If you have the correct version and it still doesn't work open an issue and I'll help you sort it out.

@euvl
Copy link
Owner

euvl commented Jul 2, 2018

Shit, i think i've published a new version to the artifactory instead of public npm >_<. My bad.

@joshualyon
Copy link

joshualyon commented Aug 15, 2018

From what I can tell, this seems to introduce issues with accessing instance properties like $store since it creates a separate root Vue instance, right? As a workaround, I understand that users could import their store directly into their individual components, but perhaps a comment should be added to the README?

I'm happy to take a stab at making an edit to the README, but wanted to confirm that instance property accessibility issues were expected/known?

@NoelDeMartin
Copy link
Contributor Author

@joshualyon I hadn't thought about that, but I'll look into it. If that's really the case I'll try to fix it instead of having to modify the README. There should be a way to make it compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants