Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support multiple grids #6

Closed
yairEO opened this issue Mar 10, 2014 · 6 comments
Closed

support multiple grids #6

yairEO opened this issue Mar 10, 2014 · 6 comments

Comments

@yairEO
Copy link

yairEO commented Mar 10, 2014

in your code, you have this piece:

if(options.resize) {
    $(window).on('resize', {container: this[0]}, function(e){

please scope your events so they wouldn't collide with others code, and change it a little to:

if(options.resize) {
    $(window).on('resize.rowGrid', function(e){
      // do 'layout' for each "this" element
}

BUT, you need to take that resize event outside, and wrap the rest of your initialization code with something like:

$.fn.rowGrid = function( options ) {
    var defaults = {
      minMargin: null,
      maxMargin: null,
      resize: true,
      lastRowClass: 'last-row',
      firstItemClass: null
    };

    return this.each(function(){
        if ( this.length == 0 ) {
          console.error( 'No element found for "' + this.selector + '".' );
          return this;
        }
        if ( this.length > 1 ) {
          return this.each(
            function() {
              $(this).rowGrid( options );
            }
          );
        }

        if(options === 'appended') {
          options = this.data('grid-options');
          var $lastRow = this.children('.' + options.lastRowClass);
          var items = $lastRow.nextAll().add($lastRow);
          layout(this[0], options, items);
        } else {
          options = $.extend( {}, defaults, options );
          this.data('grid-options', options);
          layout(this[0], options);
       }
    })

        if(options.resize) {
        $(window).on('resize.rowGrid', function(e){
            this.each(function(){
                layout(this, options);
            });
        });
    }

       ....
       ....
      // rest of the code
});



@yairEO
Copy link
Author

yairEO commented Mar 10, 2014

I would ALSO recommend to debounce your resize event and only call the layout function when the event ends. (see - http://stackoverflow.com/a/4541963/104380)

and this will maximize performance greatly, while also allowing smooth transitions to be made via CSS to the grid's items. But, you would have to give the item's parent a styled width on event layout call. do this and your plugin will rock.

brunjo added a commit that referenced this issue Mar 11, 2014
@brunjo
Copy link
Owner

brunjo commented Mar 11, 2014

Multiple grids on one page are supported.


I am not sure whether debouncing the resize event is a good idea because you have to set a width to the container and when you decrease your window size the container can be larger then the window. Then a horizontal scrollbar appears or the layout of the page is destroyed because of the too large container for a second. This is very sloppy. I don't like it.
I think it is better to improve the performance through caching, so that the resize is faster.
BTW how many people resize their browsers? ;)

@yairEO
Copy link
Author

yairEO commented Mar 11, 2014

well it's not about the actual resizing of the browser but rather the size change of the containing element. say, if you have a side menu and comes and goes, so when it comes in, the content area gets smaller.

also, I know multiple grids are supported, BUT the plugin has to be called for each element, and it's considered a good plugin authoring to do return this.each(function(){ to iterate the jQuery selector and run your code on each element, instead of calling the plugin manually for each of them. see example above.

Also this is a good read about layout performance with many DOM read/writes FYI :
http://wilsonpage.co.uk/preventing-layout-thrashing/

@brunjo
Copy link
Owner

brunjo commented Mar 11, 2014

BUT the plugin has to be called for each element

No! You can do something like this:
$(".container1, .container2").rowGrid(options);

It is done with the following code snippet:

if ( this.length > 1 ) {
  return this.each(
    function() {
       $(this).rowGrid( options );
    }
  );
}

@yairEO
Copy link
Author

yairEO commented Mar 11, 2014

well my way is the correct way and your way is a hacky one, because your way is doing hacky check to see if there are more than 1 selector and then call the plugin again, and on top of the you have 2 return statements... my way is the way everybody is doing plugins..trust me I've been writing jquery plugins since 2007 and used jQuery since before that..there is a purpose of use return this.each(function(){ ...

anyway, I wouldn't want to do $(".container1, .container2").rowGrid(options); since all my containers have the exact same class, lets say I have 3 containers with the class "container" on the page.

@brunjo
Copy link
Owner

brunjo commented Mar 11, 2014

I will change it. I trust you ;)

$(".container1, .container2").rowGrid(options); was only an example to show that the plugin works with selectors which targeting more than one element. Of course the following works, too: $(".container").rowGrid(options);

@brunjo brunjo closed this as completed in 7315dce Mar 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants