Added minRow and row #1173
Added minRow and row #1173
Conversation
src/gridstack.js
Outdated
| var idSeq = 0; | ||
|
|
||
| var GridStackEngine = function(column, onchange, float, maxRow, items) { | ||
| var GridStackEngine = function(row, column, onchange, float, minRow, maxRow, items) { |
There was a problem hiding this comment.
I do not want to have a number of rows, unlike columns which are fixed and special % CSS layout. Min/MaxRow is more than enough to get everything you need. Also maxRow is not that well supported so I don't want to expose 'row' which is a superset. Please take out.
Also adding new param to new GridStackEngine() means all the test cases that use need updating - please fix them (under spec/) and make sure npm test still works after your changes AND the demo/ apps run. I know it's a lot but something I will have to do otherwise and I don't have extra cycle for this feature right now.
There was a problem hiding this comment.
Im actually happy that it actually works. Will refactor tomorrow, I also need to get more familiar with the code. Also, is there any reasons why you use var instead of let and const?
There was a problem hiding this comment.
didn't write that legacy code! I'm actually converting to typescript (and I will have hand pick all those changes since 1.0.0 so minimal changes please!) as I hate JS spaghetti code.
src/gridstack.js
Outdated
| GridStack.prototype._updateContainerHeight = function() { | ||
| if (this.engine._batchMode) { return; } | ||
| var row = this.engine.getRow(); | ||
| var row = this.opts.minRow > this.engine.getRow() ? this.opts.minRow : this.engine.getRow(); |
There was a problem hiding this comment.
don't call methods twice! inefficient. either put the minRow in getRow() (see where it's used or here)
|
I implemented minRow how @adumesny told me to do in the first place. Also, now if |
Fastforward my repo
| GridStack.prototype._updateContainerHeight = function() { | ||
| if (this.engine._batchMode) { return; } | ||
| var row = this.engine.getRow(); | ||
| if (row < this.opts.minRow) { |
There was a problem hiding this comment.
yep, that is the right place to put it. as you can see you can also accomplish this with CSS min-height on the grid div, but setting minRow can be easier for some.
| row: parseInt(this.$el.attr('data-gs-row')) || 0, | ||
| column: parseInt(this.$el.attr('data-gs-column')) || 12, | ||
| maxRow: parseInt(this.$el.attr('data-gs-max-row')) || 0, | ||
| minRow: parseInt(this.$el.attr('data-gs-row')) ? parseInt(this.$el.attr('data-gs-row')) : parseInt(this.$el.attr('data-gs-min-row')) || 0, |
There was a problem hiding this comment.
please extract this.$el.attr('data-gs-row') into a var right above - easier to read/parse. maybe minRowAttr
|
|
||
| opts = opts || {}; | ||
|
|
||
| // if row property exists, replace minRow and maxRow |
There was a problem hiding this comment.
move that down - feel weird to be first thing we check... like right after 715 where we set opts values.
| * a string (ex: '100px', '10em', '10rem', '10%') | ||
| * 0 or null, in which case the library will not generate styles for rows. Everything must be defined in CSS files. | ||
| * `'auto'` - height will be calculated to match cell width (initial square grid). | ||
| - `row` - number of rows. This is a shortcut of writting `minRow: sameValue, maxRow: sameValue`. |
There was a problem hiding this comment.
great, but all those need to be alphabetized please. also row should say something like:
row - fixed number of rows which prevents the grid from changing as items are added/removed/moved, Shortcut for setting minRow: N, maxRow: N. default is 0 no constrain. EXPERIMENTAL - see maxRow
|
thanks, the last code version looks a lot better. I'll accept it and tweak the doc. |
Description
Added
minRowandrowproperties.minRow: Minimum amount of rows to generate, default is 0.row: Shortcut to have same value forminRowandmaxRow.fix #1172
Checklist
yarn test)