2
\$\begingroup\$

I need a code review for my first jQuery plugin. It seems like a lot of code and I want to hear something about my plugin code structure and more.

;
(function($) {
 'use strict';
 $.fn.mpopup = function(options) {
 //Default settings
 var defaults = {
 overlayClass: 'mpopup-overlay',
 overlayColor: '#000',
 overlayOpacity: 0.7,
 overlayFadeSpeed: 500,
 popupFadeSpeed: 500,
 closePopup: '.close-popup',
 trigger: '.open-mpopup',
 effect: 'fadeUp'
 };
 //Settings
 var settings = $.extend({}, defaults, options);
 //Variables
 var popup = this,
 windowWidth = $(window).width(),
 windowHeight = $(window).height(),
 popupBlockHeight = popup.outerHeight(),
 popupBlockWidth = popup.outerWidth(),
 bodyElement = $('body'),
 trigger = $(settings.trigger),
 popupHorizontalPosition = (windowWidth - popupBlockWidth) / 2,
 closeFlag = false,
 transitionEffectVal = 100,
 indent = (settings.indent) ? settings.indent : windowHeight / 6, // windowHeight / 6 for better look on resize
 isShortPopup,
 popupVerticalPosition,
 onResize,
 newPopupVerticalPosition,
 documentHeight,
 overlay = $('<div />')
 .attr('class', settings.overlayClass)
 .css({
 'top': '0',
 'bottom': '0',
 'left': '0',
 'right': '0',
 'position': 'fixed',
 'z-index': 9998,
 'display': 'none'
 });
 //Hex to RGB with opactiy
 function hex2rgb(hex, opacity) {
 var h = hex.replace('#', '');
 h = h.match(new RegExp('(.{' + h.length / 3 + '})', 'g'));
 for (var i = 0; i < h.length; i++)
 h[i] = parseInt(h[i].length == 1 ? h[i] + h[i] : h[i], 16);
 if (typeof opacity != 'undefined') h.push(opacity);
 return 'rgba(' + h.join(',') + ')';
 }
 var rgbaValue = hex2rgb(settings.overlayColor, settings.overlayOpacity);
 // Custom event for callback functions
 $.each(settings, function(key, value) {
 if (typeof value === 'function') {
 popup.on(key + '.mpopup', function() {
 value(popup);
 })
 }
 });
 //Append popup into overlay block
 function appendPopup() {
 overlay.css('background', rgbaValue).appendTo(bodyElement).append(popup);
 popup.css({
 'position': 'relative',
 'z-index': 9999
 });
 }
 //Show popup
 function triggerCall() {
 if (popup.length > 0) {
 appendPopup();
 trigger.click(function(e) {
 e.preventDefault();
 onResize = false;
 closeFlag = false;
 popup.trigger('beforeCreated.mpopup');
 shortPopup();
 longPopup();
 doEffect();
 popup.trigger('created.mpopup');
 });
 resizePopup();
 } else {
 console.log('Pop-up is undefined');
 }
 }
 // Close popup
 function close() {
 var svgCloseElem = $(settings.closePopup).find('*');
 overlay.click(function(e) {
 closeFlag = true;
 if ($(e.target).is(settings.closePopup) ||
 $(e.target).is(svgCloseElem) || // for svg elements
 $(e.target).is(overlay)) {
 overlay.css('overflow-y', 'hidden');
 // Remove overflow-y and padding-right styles from body
 documentHeight = $('html, body').height();
 if (documentHeight > windowHeight) {
 bodyElement.css({
 'overflow-y': '',
 'padding-right': ''
 });
 }
 doEffect();
 }
 popup.trigger('afterClose.mpopup');
 });
 // Close popup when clicking the esc keyboard button
 $(document).keyup(function(e) {
 if (e.which == '27') {
 overlay.removeClass('show-popup');
 popup.removeClass('show-popup');
 popup.trigger('afterClose.mpopup');
 }
 });
 }
 // If popup block height LESS than window height
 function shortPopup() {
 if (popupBlockHeight < windowHeight) {
 isShortPopup = true;
 popupVerticalPosition = (windowHeight - popupBlockHeight) / 2;
 newPopupVerticalPosition = popupVerticalPosition + transitionEffectVal;
 popup.css({
 'left': popupHorizontalPosition,
 'margin': 0,
 'top': (settings.effect === 'fadeUp' && !onResize) ? newPopupVerticalPosition : popupVerticalPosition
 })
 }
 }
 // If popup block height BIGGER or EQUAL to window height
 function longPopup() {
 if (popupBlockHeight >= windowHeight) {
 isShortPopup = false;
 popupVerticalPosition = indent;
 // Remove body scroll and add right padding equal to Scrollbar size
 documentHeight = $('html, body').height();
 if (documentHeight > windowHeight) {
 var horizontalScrollWidth = window.innerWidth - document.documentElement.clientWidth;
 bodyElement.css({
 'overflow-y': 'hidden',
 'padding-right': horizontalScrollWidth
 });
 }
 overlay.css('overflow-y', 'scroll');
 newPopupVerticalPosition = indent + transitionEffectVal;
 popup.css({
 'left': popupHorizontalPosition,
 'top': 0,
 'margin-top': (settings.effect === 'fadeUp' && !onResize) ? newPopupVerticalPosition : popupVerticalPosition,
 'margin-bottom': popupVerticalPosition
 })
 }
 }
 //Show and close effects
 function doEffect() {
 switch (settings.effect) {
 case 'fade':
 if (!closeFlag)
 overlay.fadeIn(settings.overlayFadeSpeed).find(popup).fadeIn(settings.popupFadeSpeed);
 else
 overlay.fadeOut(settings.overlayFadeSpeed).find(popup).fadeOut(settings.popupFadeSpeed);
 break;
 case 'fadeUp':
 if (!closeFlag) {
 if (isShortPopup) {
 popup.animate({
 'top': '-=' + transitionEffectVal
 }, settings.popupFadeSpeed);
 } else {
 popup.animate({
 'margin-top': '-=' + transitionEffectVal
 }, settings.popupFadeSpeed);
 }
 overlay.fadeIn({
 queue: false,
 duration: settings.overlayFadeSpeed
 }).find(popup).fadeIn({
 queue: false,
 duration: settings.popupFadeSpeed
 });
 } else {
 if (isShortPopup) {
 popup.animate({
 'top': '+=' + transitionEffectVal
 }, settings.popupFadeSpeed);
 } else {
 popup.animate({
 'margin-top': '+=' + transitionEffectVal
 }, settings.popupFadeSpeed);
 }
 overlay.fadeOut({
 queue: false,
 duration: settings.overlayFadeSpeed
 }).find(popup).fadeOut({
 queue: false,
 duration: settings.popupFadeSpeed
 });
 }
 break;
 default:
 console.log('No effect');
 }
 }
 // Popup on mobile devices
 function resizePopup() {
 $(window).resize(function() {
 onResize = true;
 windowWidth = $(window).width();
 windowHeight = $(window).height();
 popupBlockWidth = popup.outerWidth();
 popupHorizontalPosition = (windowWidth - popupBlockWidth) / 2;
 if (popupBlockHeight < windowHeight) {
 popupVerticalPosition = (windowHeight - popupBlockHeight) / 2;
 } else {
 popupVerticalPosition = windowHeight / 6;
 }
 if (popup.css('display') === 'block') {
 shortPopup();
 longPopup();
 }
 popup.trigger('onResize.mpopup');
 });
 }
 function init() {
 triggerCall();
 close();
 }
 init();
 return this;
 }
}(jQuery));

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Jul 10, 2016 at 12:30
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Please try to help reviewers by including more information about what it is your plugin actually does. \$\endgroup\$ Commented Jul 10, 2016 at 12:42
  • \$\begingroup\$ What does your popup do? Why is it special? \$\endgroup\$ Commented Jul 10, 2016 at 13:25
  • \$\begingroup\$ @forsvarir, actually, this code with demo and documentation you can find on Github \$\endgroup\$ Commented Jul 10, 2016 at 19:10
  • \$\begingroup\$ @pacmaninbw I think there is nothing special in this popup. And i made this question to improve my skills, listen smart people) Mabye i didn't find popup plugin with good documentation "for people", easy to use. Didn't find popup plugin that appears and working without little UX defects in different situations. \$\endgroup\$ Commented Jul 10, 2016 at 19:13

1 Answer 1

4
\$\begingroup\$

Mutually exclusive functions

if (popup.css('display') === 'block') {
 shortPopup();
 longPopup();
}

I don't like that you have two mutually exclusive functions. If I read the code above, I presume you open both a short and long popup. You should pull the logic that makes them mutually exclusive out of the functions, and only call one of them based on the logic:

if (popup.css('display') === 'block') {
 if (popupBlockHeight < windowHeight) {
 shortPopup();
 } else {
 longPopup();
 }
}

triggerCall() and close() and resizePopup()

The triggerCall() and close() functions do not name what they actually do: Attaching a handler to open the popup and attaching a handler to close the popup. From the name alone, I would assume resizePopup() would resize an existing popup, but instead it attaches a handler to the resize handler of the window.

I don't see a situation where you would want to be able to open a popup, but not be able to close it again. Calling any of these functions multiple times will result in wrong behaviour. I would suggest at least renaming the functions to better name what they do, but probably just remove those functions and move the attaching of handlers to the init function. It is the only time you want to call them anyway.

Misc

You have at least two calls to popup.css({ ... }), but you forgot to close the statement with a ;.

I recommend against declaring variables using the var var1, var2, var3 ... syntax. The reason for this is that it is easy to accidentally drop a variable in the global namespace by forgetting a comma. The following snippet reads weird to me, even though it is correct. If you had to define more variables, you would have continued with a comma at the end, making it even more unreadable. Consider either indenting it more, so it does not read at a glance as some more variables being declared, or move the assignment down after you declare your variables.

var popup = this,
 windowWidth = $(window).width(),
 //some more declarations
 documentHeight,
 overlay = $('<div />')
 .attr('class', settings.overlayClass)
 .css({
 'top': '0',
 'bottom': '0',
 'left': '0',
 'right': '0',
 'position': 'fixed',
 'z-index': 9998,
 'display': 'none'
 });
answered Jul 10, 2016 at 13:45
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.