search

dimsemenov / Magnific-Popup

Light and responsive lightbox script with focus on performance.
http://dimsemenov.com/plugins/magnific-popup/
MIT License
11.39k stars 3.49k forks source link

Separate style and JS classes #174

Open davidtheclark opened 11 years ago

davidtheclark commented 11 years ago

I think it would be a good idea to use separate classes for JS functionality and for style.

I ran into this problem because I wanted to style my own button, using my own markup (through the "closeMarkup" method). Instead of using the "mfp-close" class and then overriding its default styling, I just inserted my own markup, with my own class, and left "mfp-close" out. However, that broke the functionality, because in the JS (line 113, I believe), the class name "mfp-close" is hard-coded as the class for the close button's functionality.

So it seems I'm left with either using "mfp-close" and going to the trouble of overriding all of that class's styling, or changing the plugin's JS.

If there were a JS class (e.g. "js-mfp-close") independent of any styling, it could be automatically added to any custom "closeMarkup" element -- the user wouldn't even have to think about accidentally breaking functionality.

I'm sure this situation applies to all the other default elements that involve both styling and functionality.

(Or maybe I'm overlooking some existing option?)

AdamWebDev commented 11 years ago

What about using the option: 

showCloseBtn: false

which will not show the default close button, and then creating your own close button in the modal. You can style it however you like, then use a javascript click event to close it using:

var magnificPopup = $.magnificPopup.instance;
magnificPopup.close();
davidtheclark commented 11 years ago

Thanks -- that would work, without modifying the plugin JS. But I don't think that the existence of the .close() method mitigates the problem caused by the mixing of presentational and functional classes here. It certainly makes the "closeMarkup" option a little silly, since the element I create won't actually close the popup but instead has to be manually assigned (just like any other element).

Besides avoiding issues like the one I described above, creating functional classes would also simplify customization by giving users a specific class they could apply to any element to trigger specific plugin functionality, without the need to manually assign that function to that element. For example, I might create a button at the bottom of my text-based popup saying "I have read this", and simply by assigning the class "js-mfp-close" (which would not alter its styling) I know that my close button will work with the plugin.

AdamWebDev commented 11 years ago

I do agree... before getting into the documentation, my first instinct was to simply assign the "js-mfp-close" class to my custom "Cancel" button - which definitely doesn't give you the intended result.

dimsemenov commented 11 years ago

Sorry for late reply, guys. Magnific Popup stylesheet is dynamic, so I'm assuming that if you wish to modify styling of close button - you just delete/change the default styling from the popup stylesheet https://github.com/dimsemenov/Magnific-Popup/blob/master/src/css/main.scss#L237

BenRacicot commented 10 years ago

I also ran into this problem with : var magnificPopup = $.magnificPopup.instance; $('a.close').magnificPopup.close();

It works but seems to clear the content of any other popup so that the next time a popup is opened its content is not appearing. I had to position and style the native mfp-close button which is not as classy as the rest of the functionality on magnifPopup.