josecebe / twbs-pagination

jQuery pagination plugin (bootstrap powered)
http://josecebe.github.io/twbs-pagination/
Other
1.1k stars 401 forks source link

Improved "setupEvents" method #75

Closed yairEO closed 7 years ago

yairEO commented 8 years ago

It's better to use event delegation than setting up the same event for each li element.

This is what I changed it to (I will not use a pull request, but instead you can just copy-paste)

setupEvents: function () {
            var base = this;

            this.$listContainer.off().on('click', 'li', function(e){
                var $this = $(this),
                    pageNumber = +$this.data('page');

                if( $this.hasClass(base.options.disabledClass) || $this.hasClass(base.options.activeClass) ){
                    return false;
                }

                // Prevent click event if href is not set.
                !base.options.href && e.preventDefault();
                base.show( pageNumber );
            });
}

Event delegation is always better in term of memory than attaching an event on single elements. Also this method is less code, and is faster, because there is no need to do each() and then attach the same event on each of the items. Also, I am using the on method instead of directly calling click which is considered better practice in terms of consistency (in the jQuery world)

eugenesimakin commented 7 years ago

Done