Skip to content

Conversation

@adumesny
Copy link
Member

Description

*method was not correctly counting margin when figuring out minHeight during a drag
(see #999)

  • fixed cellHeight() getter to correctly returns the value and checked all places that use to adjust for margins

Checklist

  • All tests passing (yarn test)
  • Extended the README / documentation, if necessary

I tested two.html as well as my app that doesn't a lot of drag&drop and made sure things resized correctly and no new issue, but min size is now correct (see #999 screen shots)

Alain Dumesny added 2 commits November 11, 2019 15:40
*method was not correctly counting margin when figuring out minHeight during a drag
(see #999)
* fixed cellHeight() getter to correctly returns the value and checked all places that use to adjust for margins
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 65.583% when pulling 6b07c28 on adumesny:bugfix/999 into 66339a5 on gridstack:develop.

@radiolips
Copy link
Member

opts.verticalMargin can be a string, so those references will need to be changed to something that returns a number.

@adumesny
Copy link
Member Author

adumesny commented Nov 13, 2019

@radiolips I changed one of the demo (two.html) to have verticalMargin: "2rem" in the opts and sure enough gridstack constructor calls this.verticalMargin(this.opts.verticalMargin, true); and the only other way to set is to call verticalMargin(val) directly - in there we do extract the number vs the unit so opts.verticalMargin will NEVER store a string.

Now mind you we fail to convert the units to actual pixels so '2rem' = '2px' = '2em' = 2 which is clearly incorrect, but that's a separate issue (maybe the CSS is update for other values, but verticalMargin is pixel code and therefore wrong.

@radiolips radiolips merged commit 02b8154 into gridstack:develop Nov 13, 2019
@adumesny adumesny deleted the bugfix/999 branch November 17, 2019 18:54
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.

3 participants