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

Bugs with new fitToContent (1rem, calling cellHeight) #2427

Closed
ExusAltimus opened this issue Aug 29, 2023 · 5 comments · Fixed by #2563 · May be fixed by Samg217/glpi#8
Closed

Bugs with new fitToContent (1rem, calling cellHeight) #2427

ExusAltimus opened this issue Aug 29, 2023 · 5 comments · Fixed by #2563 · May be fixed by Samg217/glpi#8
Labels

Comments

@ExusAltimus
Copy link

ExusAltimus commented Aug 29, 2023

Subject of the issue

Issue 1: fitToContent / misbehaving not working when cellHeight is not an integer (1rem support) FIXED

Issue 2: Starting with column 2, then changing the cellHeight, then changing the column to 1 will revert it back to 2 column

Issue 3: Changing cellHeight will not auto size the cell to content height
NOTE: This may not be a bug and may be intended behavior, i.e call resizeToContent on cellHeight() call
FIXED

Issue 4: Clicking add widget will not auto size the cell to the content height (added makeWidget version too because this is my typical flow)
NOTE: This may not be a bug and may be intended behavior, i.e call resizeToContent on widget add (addWidget or makeWidget)
FIXED

Your environment

Gridstack Version 9.0.1 (using gridstack.extra.css)
Google Chrome, Edge

Steps to reproduce

https://jsfiddle.net/zxwg5j7m/101/
Issue 1 + 3
36b59a30de7c6c75bfe704f54d8347a9

Issue 2
4000c84651a8817f6bbd5c04b1ea4c41

Issue 4
Image from Gyazo

Expected behavior

Cell should fit to content,
addWidget / makeWidget should auto size the cell
cellHeight should auto size the cell

@adumesny
Copy link
Member

adumesny commented Aug 29, 2023

not surprised 1rem doesn't work. as for 3 & 4 changing cellHeight (with a px number) and adding widget/makeWidget should call resizeToContent(). that's a bug.

Funny the only time I head from people is when a big new feature has bugs and people apparently start using it right away...
#404 has been 7 years in the making :)

@adumesny adumesny changed the title Bugs with new fitToContent feature Bugs with new fitToContent (rem, calling cellHeight, addWidget/MakeWidget) Aug 29, 2023
@ExusAltimus
Copy link
Author

This is by far the best project I have found for dashboard-style widgets, other libraries don't have nearly all the features that this one does. I was also using a custom solution for resizing right before this came out so I decided to swap over and try it out. I've been slowly getting familiar with the code base so next time I run into something I'll put some effort into looking into a fix

@adumesny
Copy link
Member

yeah, fitToContent is something my current project at work was needing (all previous were stretch to fit gridItem instead as mostly graphs) and after having it solved there I decided move the code to GS as I will possibly make live resizing be aware of fitToContent.

I actually need to support having a user size vs fitting to content as sometimes content is smaller so no need to waste space, but them max at user settable size when the content is too big (dynamic content), so big new feature still comming...

adumesny added a commit to adumesny/gridstack.js that referenced this issue Aug 29, 2023
* partial fix for gridstack#2427
* when changing cellHeight, or calling addWidget() | makeWidget() we now call doContentResize()
* fixed doContentResize logic to use expected size (using cellHeight) instead of actual DOM values so we don't need to delay until animation is done (unlike column width which does affect calc height for content reflow)

TODO: support 1rem type of cellHeight.
adumesny added a commit to adumesny/gridstack.js that referenced this issue Aug 29, 2023
* partial fix for gridstack#2427
* when changing cellHeight, or calling addWidget() | makeWidget() we now call doContentResize()
* fixed doContentResize logic to use expected size (using cellHeight) instead of actual DOM values so we don't need to delay until animation is done (unlike column width which does affect calc height for content reflow)

TODO: support 1rem type of cellHeight.
@adumesny
Copy link
Member

adumesny commented Aug 29, 2023

item 3 and 4 are now fixed. others will have to wait...

@adumesny adumesny changed the title Bugs with new fitToContent (rem, calling cellHeight, addWidget/MakeWidget) Bugs with new fitToContent (1rem, calling cellHeight) Oct 23, 2023
adumesny added a commit to adumesny/gridstack.js that referenced this issue Dec 10, 2023
* fix gridstack#2427
* getCellHeight() now support rem/em natively (before items are sized correctly)
@adumesny
Copy link
Member

adumesny commented Dec 10, 2023

closing. # 2 is separate issue and should be filed if still present.
Don't forget to donate if you find this lib useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants