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

Modalizing #2

Closed
rickydazla opened this issue Jun 30, 2014 · 14 comments
Closed

Modalizing #2

rickydazla opened this issue Jun 30, 2014 · 14 comments

Comments

@rickydazla
Copy link
Member

rickydazla commented Jun 30, 2014

Hey @jayvin I am running into some inconsistent issues, I think caused by some confusion between me / you / @trajce as to how these popups (should) work...

My initial vision was for the CSS modalHolder to have display:none and the script would then add the "modalize" class. The modalize class has display block so that would show the modal and then a bunch of other associated attributes would then apply to child elements.

However, I think the script is now using show() and hide() as I see that in various implementations there is an inline display:none and / or the modalize class is already applied. I still see refs to "modalize" in the popup script.

Can you clarify what is happening and/or fix it up? I would like the script to add/remove the modalize class. However, I see the benefit of show/hide as it can be used to fade the modal instead of just boshing it in. Can we do a combo of both? i.e. script adds "modalize" and then fades it in? Maybe we can add options for fade true/false and if true a setting for speed of fade...

@jayvin
Copy link
Contributor

jayvin commented Jun 30, 2014

@rickydazla yes I saw this too. Its because of some stores we are using show and on some the modalize class. Well we can add both of this. Let me check what's the best way we can do this.

@jayvin
Copy link
Contributor

jayvin commented Jul 1, 2014

@rickydazla : Hey man, since we are loading the jquery.form-n-validate.js plugin from the store itself instead of the Mailchimp cdn, I can do some further clean ups to optimize speed. What store can I work to test the changes??

@rickydazla
Copy link
Member Author

Use your old friend zboncak : )

@jayvin
Copy link
Contributor

jayvin commented Jul 31, 2014

Hey man, while working on ballandbuck, I updated the plugin(both the popup and embed). We do not need the jquery.form-n-validate.js plugin anymore and relevant codes which were loading this plugin has been removed, popup will load more faster now.

I was thinking about doing working on your previous comment above directly on ballandbuck, but their popup css and html is very different from what it should be. So I will work on this directly on zboncak tomorrow.

@jayvin
Copy link
Contributor

jayvin commented Aug 1, 2014

@rickydazla : Hey man, before proceeding with the changes, I was thinking maybe we can merge the snippet mailchimp-signup-script.liquid and mailchimp.popup.js.liquid into one single file. Technically there is no problem but I wanted to your opinion before.

@rickydazla
Copy link
Member Author

@jayvin I know this is old, but just to respond anyway in case I haven't on Asana... why not?

@rickydazla
Copy link
Member Author

I think previously we were doing some config in the snippets/mailchimp-signup-script.liquid file but now it is all abstracted to config/settings.html... As I recall, we had originally copied the mailchimp.popup.js.liquid file directly from MailChimp just to have in on the same server but now you have hacked it some so it all belongs to us!

@jayvin
Copy link
Contributor

jayvin commented Feb 10, 2015

@rickydazla, coming to this one, from your first comment.
Currently, the form becomes visible like this
setTimeout(function () {
T3MCP.Popup.fadeIn().addClass('modalize');
_MCPopupCustomCallBack.OnShow();
}, delay);

The add class 'modalize' was not there, I have just added it.
For hiding, default jquery hide is called.

Is that the behavior that you wanted, if not let me know, I will do the changes.

@rickydazla
Copy link
Member Author

Seems like .attr('hidden',true) would be better, see Paul Irish's discussion here re: performance of hide(). Although, with that said, I would also like to add/remove the class modalize since that is how I originally wrote the CSS...

@jayvin
Copy link
Contributor

jayvin commented Feb 11, 2015

For the hide, I will just remove the modalize class.
What about show? Should we fadeIn the popup as well or just modalize it?

@rickydazla
Copy link
Member Author

Just add modalize and attr('hidden',false). No effects just 💣

@rickydazla
Copy link
Member Author

(*) I have noticed that on some of our sites the modal fades in before the background images has loaded...

@jayvin
Copy link
Contributor

jayvin commented Feb 11, 2015

Yes, actually I know why this fading thing is there, I remember some client wanted to have a fading effect and since then, this piece of code has remained into the plugin. :)

jayvin added a commit that referenced this issue Feb 12, 2015
@jayvin
Copy link
Contributor

jayvin commented Feb 12, 2015

Done, removed fadeIn effect and popup will hide by removeClass 'modalize' instead of .hide() function..

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

No branches or pull requests

2 participants