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

no need to "float" items #1

Closed
yairEO opened this issue Feb 20, 2014 · 4 comments
Closed

no need to "float" items #1

yairEO opened this issue Feb 20, 2014 · 4 comments

Comments

@yairEO
Copy link

yairEO commented Feb 20, 2014

Using floats (in this manner) is considered bad practice. Please use inline-block and if your HTML is not minified, then you will have to hack the container with font-size:0.

@brunjo
Copy link
Owner

brunjo commented Mar 4, 2014

Why is it a bad practice? I do not see the point. The performance of floats is even a little better: https://github.com/mdo/css-perf#grid-techniques.
I think it is a matter of opinion if float or inline-block is better. What do you think?

@yairEO
Copy link
Author

yairEO commented Mar 7, 2014

float's need to be cleared, and floats cannot be centered. If you think about it, float is something that needs to be floated next to a non-floated element, to be out-of-context. The condition best describing the semantics of your grid's items is inline-block which is what each item, a box which is inlined with other boxes. float should only be used where no other display solution can be achieved.

In another subject, what would be the performance of your gallery when dealing with thousands of items? The javascript is not designed to handle large amounts of items, since there no optimizations for performance..the key is to cache items, access the DOM as few times as possible and consider bulk updating and replacing the whole container's html every time. also it's very important to NOT use jQuery where ever possible, since it's very slow.

  • using $container.children on every resize is bad..cache items and add/remove items when DOM changes.
  • items.removeAttr('style').removeClass(options.firstItemClass); - using jQuery functions for each item is slow, try doing a bulk change.
  • items.each(.. - again, very slow. replace this with a normal loop which is MUCH faster.
  • elem.outerWidth() and elem.height() using pure javascript will increase performance by far, since this is called for each item a jQuery function is called, then that function is doing it's thing. a function call is always expensive, and you can do that same thing directly with JS..
  • rowElems[rowElemIndex].css('margin-right', ... just use normal DOM style method. much faster. using the .css jQuery is only recommended when manipulating small amount of elements, so this is not performance-scalable.

I want to say that I really love your plugin, it's really, truly, a good one!

@brunjo
Copy link
Owner

brunjo commented Mar 7, 2014

Thanks yairEO, I appreciate your advices.

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

brunjo commented Sep 5, 2015

FYI the plugin does not require a float grid system you can also use display: inline-block without problems.

@brunjo brunjo closed this as completed Sep 5, 2015
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