Skip to content

Bug fix: Init with marginTop prop or other margin props will turn height to 1 when resize a grid#1441

Closed
deadivan wants to merge 2 commits into
gridstack:developfrom
deadivan:develop
Closed

Bug fix: Init with marginTop prop or other margin props will turn height to 1 when resize a grid#1441
deadivan wants to merge 2 commits into
gridstack:developfrom
deadivan:develop

Conversation

@deadivan

@deadivan deadivan commented Nov 5, 2020

Copy link
Copy Markdown

Description

Bug fix: Init with marginTop prop or other margin props will turn height to 1 when resize a grid

Checklist

  • Created tests which fail without the change (if possible)
  • All tests passing (yarn test)
  • Extended the README / documentation, if necessary

Bug fix: Init with marginTop prop or other margin props will turn height to 1 when resize a grid
Comment thread src/gridstack.ts
}

//without this.opts.margin, getMargin method will return undefined, and affect resize behavior
if(!this.opts.margin) this.opts.margin = Math.round((this.opts.marginTop + this.opts.marginBottom) / 2);

@adumesny adumesny Nov 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't the right fix because margin=0 is a valid value. Thanks for bringing this issue up, will fix it the proper way next...

adumesny added a commit to adumesny/gridstack.js that referenced this pull request Nov 5, 2020
* better fix than gridstack#1441 - thanks for suggestion!
* when reisizng we incorrectly use getMargin() which is correctly 'undefined' if all 4 sides don't match
(should had bee using top+bottom but actually  doesn't need any margin, like horizontal)
* fiuxed test cases to check for these scenarios
@adumesny

adumesny commented Nov 5, 2020

Copy link
Copy Markdown
Member

thanks for the suggestion! but ended up checking a correct fix.

@adumesny adumesny closed this Nov 5, 2020
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

Successfully merging this pull request may close these issues.

2 participants