Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

About modal rendering #7

Open
RedRelay opened this issue Jan 31, 2018 · 1 comment
Open

About modal rendering #7

RedRelay opened this issue Jan 31, 2018 · 1 comment

Comments

@RedRelay
Copy link
Contributor

Hi,
I do not have a lot of experience in meteor or materialize framework and I have some questions about your code before creating futher more pull request. Moreover I do not have any experience on open source contributing.

I've seen you cut modalParentId from cloned template data object into template instance object in launcher.js onCreated(). I do not understand why you clone data and put it to instance directly instead of using instance.data or Template.currentData() directly. Is it for performance reasons ?

Futhermore, I got confusing with the use of $('#'+instance.data.id).parents('.container') to get qModalParent in case of modalParentId is undefined.
It seems $('#'+instance.data.id) will always returns no element, because at this point, modal is not rendered.

At last, if modal is already rendered we remove it before rendering it again.
Are you sure the modal id is '#'+instance.id ? I do not found the definition of instance.id.
However I think '#'+instance.modalId is more accurate.
Could you tell me if that is a bug/miss as I expect or not ?

@mozfet
Copy link
Owner

mozfet commented Jan 31, 2018

No worries about your experience, any help is appreciated. This a large suite of components and it is a lot of work keeping it up all by myself.

The clone is to prevent modifying the underlying data, which would be caused by the delete of the modalParentId key. If the underlying data is changed, it would start interfering when you try to launch the same modal twice. Removing the modalParentId is to prevent the autoform data becoming corrupted with data that should not be there (I'm not sure if it causes errors, but the field is certainly not supported by autoform).

Furthermore, it is best practice to store data for the instance on the instance, instance.data should only be used for data passed to the template instance from a parent template.

I have not recently tried it without a modalParentId, but can tell you that materialize is very finicky about where a modal container is placed and it will show in the wrong place or show corrupted if nested within some incompatible divs; for that reason I usually define a div in the body or materialize container to hook all modals to, and then pass that div's id to the modal launcher to avoid hooking it on things like form fields or hidden buttons. If you do not provide a modal parent id, I try to find the nearest container to hook the modals on.

Lastly regarding qModalParentNode.find('#'+instance.id); I think you are right, that looks like a bug.

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

No branches or pull requests

2 participants