-
Notifications
You must be signed in to change notification settings - Fork 13
add suport for custom css classes for columns #31
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @revyh thanks for this, greatly appreciated. I've a single comment regarding the code, can you review?
src/Grid.js
Outdated
@@ -165,7 +169,7 @@ Grid.prototype.redraw = function(newOptions, callback) { | |||
var items; | |||
|
|||
window.requestAnimationFrame(function() { | |||
if (this.columns !== newOptions.columns) { | |||
if (this.columns !== newOptions.columns || this.columnClasses !== newOptions.columnClasses) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed here. Is there a use case where columns need to be redrawn to the same number of columns as before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did this for cases when only column classes change, not their number:
savvior.init("#myGrid", {
"screen and (max-width: 20em)": {
columns: 2,
columnClasses: 'columns columns__some-modificator'
},
"screen and (min-width: 20em) and (max-width: 40em)": {
columns: 2,
columnClasses: 'columns columns__some-other-modificator'
},
});
Of course, it's not very useful config since it can be easily replaced with css:
@media screen and (max-width: 20em) {
.columns__some-modificator { /* some rules */ }
}
@media screen and (min-width: 20em) and (max-width: 40em) {
.columns__some-modificator { /* some other rules */ }
}
But such configs are possible and I think user's wtf/minute rate will increase if column classes don't change from columns columns__some-modificator
to columns columns__some-other-modificator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A redraw is a fairly expensive operation and it's best if done only when the number of columns actually change. I think this could lead to unnecessary redraws – especially, as you pointed out above, it can be solved easily with changes in CSS only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that redraws may be quite expensive. But they occurs rarely (not on every mousemove
event for example). So my first intension was to not complicate logic with edge cases and always do redraws. If you wish, I can optimise this by changing css classes only instead of full redraw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you stated before, this can, and I believe it should, be controlled by CSS using media queries so why add the complexity here.
I am happy to merge without this feature. 👍
new option for custom css classes