2
\$\begingroup\$

I'm working now on project, which is need some read more functionality and I tried to write this jquery plugin and share it in my github account.

Here is the full listing of code:

;(function ( $, window, document, undefined ) {

function get_link(readmore_wrap_class, readmore_class, readmore_text ) {
    return $('<a/>')
                .addClass(readmore_wrap_class)
                .prop('href', '#')
                .html(
                    $('<span/>')
                        .addClass(readmore_class)
                        .text(readmore_text)
                    );
}

function get_first_p(text, sentences_number) {
    var paragraph = text.find('p:first-child').clone();

    return paragraph
            .text(
                paragraph
                    .text().split('.', sentences_number)
                    .join('. ') + '. '
            );

}

$.fn.readmore = function (options) {
    var settings = $.extend({
        sentences_number: 3,
        readmore_text: 'Read more.',
        readmore_toggle_text: 'Read less.',
        readmore_wrap_class: 'readmore_link_wrap',
        readmore_class: 'readmore_link',
        bidirectional: false
    }, options);

    return this.each(function(index) {

        var text = $(this),

            // replace default settings with per text data attributes
            sentences_number     = text.data('sentences_number')     || settings.sentences_number,
            readmore_text        = text.data('readmore_text')        || settings.readmore_text,
            readmore_toggle_text = text.data('readmore_toggle_text') || settings.readmore_toggle_text,
            readmore_wrap_class  = text.data('readmore_wrap_class')  || settings.readmore_wrap_class,
            readmore_class       = text.data('readmore_class')       || settings.readmore_class,
            bidirectional        = text.data('bidirectional')        || settings.bidirectional;

            link = get_link(readmore_wrap_class, readmore_class, readmore_text),
            first_p = get_first_p(text, sentences_number);

        /*
        Copy first paragraph
        Save only first `sentences_number` sentences
        Add link `read more`
        Hide Original paragraphs
        On click on `read more` link delete rirst extra paragraph
        Show original paragraphs
        if bidirectional
            add 'readless' link to whole text
            On click to it delete it and run readmore again (go to 1 step)
         */
        text.find('p').hide();

        link
            .appendTo(first_p)
            .on('click', function(event) {
                first_p.remove();
                text.find('p').show();


                if (bidirectional) {
                    get_link(readmore_wrap_class, readmore_class, readmore_toggle_text)
                        .appendTo(text)
                        .on('click', function(event) {
                            $(this).remove();
                            text.readmore();

                            event.preventDefault();
                        });
                }

                event.preventDefault();
            });


        text.prepend(first_p);
    });
};

($('.readmore').length !== 0) && $('.readmore').readmore();

// end of jquery plugin wrap
})( jQuery, window, document );

Codereview goals:

  • What I have missed?
  • What can I improve?
\$\endgroup\$
  • \$\begingroup\$ Is splitting on sentences really what you need? Other options are word count, letter count(but round on words), lines(use line-height * lines, is tricky) And in all cases I would not just get the text from the first paragraph because very often there is 1 paragraph per line. \$\endgroup\$ – René Oct 5 '12 at 9:59
  • \$\begingroup\$ Don't forget to remove console.log! \$\endgroup\$ – Larry Battle Oct 8 '12 at 15:57
  • \$\begingroup\$ @LarryBattle Thank you, but I have not such commands in code) \$\endgroup\$ – Vladimir Starkov Oct 8 '12 at 17:03
  • \$\begingroup\$ @VladimirStarkov Check out line 90 here \$\endgroup\$ – Larry Battle Oct 8 '12 at 22:40
2
\$\begingroup\$

The code is good, only a few ignorable pointers :

  • lowerCamelCase is the standard for JavaScript, so

    • get_link -> getLink
    • get_first_p -> getFirstParagraph
    • readmore -> readMore ?
  • The code could be more DRY for the retrieval of settings. I would counter-propose something like this:

    var defaultSettings = {
        sentences_number: 3,
        readMoreText: 'Read more.',
        readMoreToggle_text: 'Read less.',
        readMoreWrapClass: 'readmore_link_wrap',
        readMoreClass: 'readmore_link',
        bidirectional: false
    };

   var settings = $.extend({}, defaultSettings, options);

   return this.each(function(index) {

        var text = $(this),
        var elementSettings = {};

        //Optionally replace settings with per text data attributes
        for( key in defaultSettings )
          elementSettings[key] = text.data( key ) || settings[key];

        //Then you could either
        link = get_link( elementSettings ),
        //Or you could
        link = get_link(elementSettings.readmore_wrap_class, 
                        elementSettings.readmore_class, 
                        elementSettings.readmore_text);
  • ($('.readmore').length !== 0) && $('.readmore').readmore(); this is a bit too Golfic ( reminds me of code golf ) and has a repeated constant which should be extracted and part of your readMore object in my mind.

  • Finally, your commenting is very un-even, one part has a lot, the rest has close to nothing. I'd say that your functions deserve each a /* One liner */.

| improve this answer | |
\$\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.