2
\$\begingroup\$

I have written some code which makes a button morph into a container. I have written it using prototypes. I am fairly new to jQuery/JS so was looking for advice on whether this was a bad/good way to write the script?

A previous version of my implementation was reviewed here.

I was just thinking, would it have been more appropriate to have just had the constructer function Morphing and then an object containing all the other methods and things, rather than loads of different prototypes?

Here is the jQuery:

function Morphing( button, container, content, span, top) {
    this.button = button;
    this.container = container;
    this.content = content;
    this.overlay = $('div.overlay');
    this.span = span;
    this.top = top;

    var self = this;

    this.positions = {

        endPosition: {
            top: Morphing.top,
            left: '50%',
            width: 600,
            height: 400,
            marginLeft: -300
        },

        startPosition: {
            top: self.container.css('top'),
            left: self.container.css('left'),
            width: self.container.css('width'),
            height: self.container.css('height'),
            marginLeft: self.container.css('margin-left')
        }
    };

}

Morphing.prototype.startMorph = function() {
    var self = this;
    this.button.on('click', function() {
        $(this).fadeOut(200);
        console.log('Button clicked, button faded out');
        setTimeout(self.containerMove.bind(self), 200);
    });
};

// Perhaps the rest of the code under should just be in a normal object?

Morphing.prototype.containerMove = function() {
    var self = this;
    this.overlay.fadeIn();
    this.container.addClass('active');
    console.log('Overlay shown, container given active class');

    this.container.animate(this.positions.endPosition, 400, function() {
            self.content.fadeIn();
            self.span.fadeIn();
            console.log('Container animated to center, content and span shown');
            self.close();
    });
};

Morphing.prototype.close = function() {
    var self = this;
    this.span.one('click', function() {
        self.content.fadeOut();
        self.span.fadeOut();
        self.overlay.fadeOut();
        console.log('Span clicked. Content, span, overlay all hidden');
        setTimeout(self.animateBack.bind(self), 275);
    });
};

Morphing.prototype.animateBack = function() {
    var self = this;
    this.container.animate(this.positions.startPosition, 400, function() {
        self.button.fadeIn(300);
        self.container.removeClass('active');
        console.log('Container animated back to start. Button shown and container removed active class');
    });
};

And the index.html:

    <body>

        <button class="morphButton">Terms & Conditions</button>

        <div class="morphContainer">
            <span class="close">X</span>
            <h1 class="content">Terms & Conditions </h1>
            <p class="content"> Pea horseradish azuki bean lettuce avocado asparagus okra. Kohlrabi radish okra azuki bean corn fava bean mustard tigernut juccama green bean celtuce collard greens avocado quandong fennel gumbo black-eyed pea. Grape silver beet watercress potato tigernut corn groundnut. Chickweed okra pea winter purslane coriander yarrow sweet pepper radish garlic brussels sprout groundnut summer purslane earthnut pea tomato spring onion azuki bean gourd. </p>
        </div>

        <button class="newButton">New</button>

        <div class="newContainer">
            <span class="newClose">X</span>
            <h1 class="newContent">New Stuff</h1>
            <p class="newContent">Pea horseradish azuki bean lettuce avocado asparagus okra. Kohlrabi radish okra azuki bean corn fava bean mustard tigernut juccama green bean celtuce collard greens avocado quandong fennel gumbo black-eyed pea. Grape silver beet watercress potato tigernut corn groundnut. Chickweed okra pea winter purslane coriander yarrow sweet pepper radish garlic brussels sprout groundnut summer purslane earthnut pea tomato spring onion azuki bean gourd.</p>
        </div>




        <div class="overlay"></div>

<script>
$(document).ready(function() {

    var morph = new Morphing( $('button.morphButton'), $('div.morphContainer'), $('h1.content, p.content'), $('span.close'), 100 );

    var morphTwo = new Morphing( $('button.newButton'), $('div.newContainer'), $('h1.newContent, p.newContent'), $('span.newClose'), 200 );


    morph.startMorph();
    morphTwo.startMorph();

});

</script>

    </body>

jsfiddle: https://jsfiddle.net/Specksavers/9a2projy/2/

The console.log() statements will be removed in the final version

\$\endgroup\$
  • 1
    \$\begingroup\$ This question is similar to the previous iteration but, for the moment at least, is a reasonable follow on. I have voted to leave-open based on it not being a clear duplicate. If the code has other technical (not working) issues, please let me know., \$\endgroup\$ – rolfl Mar 26 '15 at 14:44
1
\$\begingroup\$

To the specific question posed, I see no issue with the way you are assigning functions to the prototype. The only thing to consider is if they should be callable from the calling code. Some or all of the functions may be better as internal functions only callable from within the object.

I did notice a few other issues/suggestions, one of which appears to be a bug.

You have the following in the endPositions object:

top: Morphing.top,

The problem is that Morphing.top is undefined. I believe you meant self.top.

This then brought me to the fact that your use of self here is unnecessary. In this context--as well as in the startPosition object--you can simply use this or in fact omit the prefix entirely.

The following 3 lines are the same in this context:

top: self.top
top: this.top
top: top

Some other minor suggestions:

  • top is only ever used as part of endPosition. You don't really need to add it to the Morphing object at all. (unless of course you have other reasons for wanting it to be accessible)
  • startMorph seems to me to be more of an "init" than really a start function. I don't see much use in making it a separate function that needs to be called. You could call it directly from the constructor. This way the calling code only has to call the constructor and not also call the start function. (Also to consider, what would happen if startMorph were to be called twice?)
  • You could inject overlay into the page the first time it is needed or in the first call to the constructor so that pages using this feature don't have to add the overlay into their markup.
  • You might consider using a class or data attribute to indicate which elements are content or close elements rather than passing them into the constructor. For content, it might even make sense to consider all children of the container to be content unless indicated otherwise in some way. As it is now, it's possible to pass elements in as content which are not even contained in the container. This seems a bit odd.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.