jquery.readmore plugin

Question

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?

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. — René
– René, Commented Oct 5, 2012 at 9:59
@LarryBattle Thank you, but I have not such commands in code) — Vladimir Starkov
– Vladimir Starkov, Commented Oct 8, 2012 at 17:03

konijn · Accepted Answer · 2013-12-23 18:49:19Z

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 */.

Stack Exchange Network

jquery.readmore plugin

Codereview goals:

1 Answer 1

You must log in to answer this question.

Hot Network Questions

jquery.readmore plugin

Codereview goals:

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions