panes icon indicating copy to clipboard operation
panes copied to clipboard

[BUG] calcFitHeight calculates wrong value

Open AE1NS opened this issue 2 years ago • 6 comments

Describe the bug When using the fitHeight option, changing the drawer content from 200px to 100px and call calcFitHeight(), the content grows instead of shrinking.

To Reproduce https://jsfiddle.net/jwe3r4uy/

Expected behavior When changing the content from 200px to 100px, the drawer should shrink.

AE1NS avatar Sep 06 '22 09:09 AE1NS

Do you think there is any workaround till its fixed? When I see it correctly, its caused by an interal variable that holds the previous value and is added as 'diff'. So currently I dont see a possible workaround, but maybe you have an idea.

AE1NS avatar Sep 07 '22 05:09 AE1NS

@AE1NS ill check it in few day, thank you for issue

roman-rr avatar Sep 07 '22 08:09 roman-rr

@AE1NS Please check version from master branch. If that's fixed i gonna push the tag version up.

roman-rr avatar Sep 11 '22 06:09 roman-rr

Just tested it. Looks good, thank you!

AE1NS avatar Sep 12 '22 05:09 AE1NS

Unfortunately there is another issue now. When you change the content height to more than 100% of the pane and change it back, the calculated height is wrong (in the example it just moves out of the window at the bottom). Here is an example: https://jsfiddle.net/0oy71j2c/

AE1NS avatar Sep 13 '22 06:09 AE1NS

@AE1NS big thank you for your support. Should be fixed and pushed v1.3.13. FitHeight logic quite fresh and I wouldn't able to test all cases, do not hesitate to create new tickers if so. Maybe it is a good point to create automated screenshot testing with cypress, to reduce such cases.

roman-rr avatar Sep 13 '22 14:09 roman-rr

Unfortunately there seems still to be issues with calcFitHeight.

Could you have a look to this example: https://jsfiddle.net/nzb4gphe/

At the end, the container does not have the correct height, its larger.

AE1NS avatar Jun 29 '23 08:06 AE1NS

@AE1NS Thank you for report. Does it happens after recent tag release v.1.3.31 ?

roman-rr avatar Jun 29 '23 17:06 roman-rr

I dont know exactly. We used 1.3.21 and I had problems with wrong pane heights. I had situations, where the pane sometimes had a wrong height and calcFitHeight didnt solve it. Thats why I upgraded to latest and created the jsfiddle based on it. But I dont know if its the same cause of the problem. When I had the problem in our app, I had to remove the height values of the wrapper container and then calcFitHeight fixed it, but with ugly animations because some bottom shaddow of a wrapper container was visible while animation.

AE1NS avatar Jun 30 '23 04:06 AE1NS

@AE1NS Ok I'll re-check.

roman-rr avatar Jun 30 '23 06:06 roman-rr

I just see thats its even more problematic on 1.3.31 than on 1.3.21. With 1.3.31 I have more cases where the pane is not calculated correctly. In my case the pane gets not shrinked and stays at the full previous height.

AE1NS avatar Jul 06 '23 05:07 AE1NS

@AE1NS Please check your example and also https://jsfiddle.net/aenfx4gc/ Looks like all cases are covered properly.

roman-rr avatar Jul 09 '23 18:07 roman-rr

Thanks for the update! Could you have a look at my jsfiddle example again. Its technically working now, but shouldnt be the pane still be 100% height when the animation starts (as it worked before)? Now the container 'jumps' small to 100px and animates down afterwards. Thats the same effect as in the previous version, when I just removed the 'height' property and called calcFitHeight and looks really strange as I also noted in my last comment (with the bottom shadow).

AE1NS avatar Jul 10 '23 04:07 AE1NS

@AE1NS You are absolutely right. I will improve transitions to be a smooth.

roman-rr avatar Jul 10 '23 10:07 roman-rr

@AE1NS Please check.

roman-rr avatar Jul 11 '23 12:07 roman-rr

There are still problems, the button is pushed out at the bottom here: https://jsfiddle.net/ezmct1L5/

The height should also be handled correctly, when the rule * { box-sizing: border-box; } is set for the application, because the wrapper will change from 115px to 100px then and the child stays at 100px.

AE1NS avatar Jul 12 '23 05:07 AE1NS

@AE1NS Checked all your cases, reviewed algorithm and seems like works now for all. Thanks for your big help on improvements. Please check.

roman-rr avatar Jul 16 '23 17:07 roman-rr

There seems to be another strange behavior: https://jsfiddle.net/8dy0wjt2/ The last container has a wrong height. If calcFitHeight is called after another second its correctly, buts it seems like another bug no?

AE1NS avatar Jul 18 '23 08:07 AE1NS

In my use case in my app, I also have two steps/times I call calcFitHeight, when the content has the same size. I just see that the pane element sets a 'height' css value and doesnt remove it. In the other cases when the height changes, it always gets removed automatically.

AE1NS avatar Jul 18 '23 11:07 AE1NS

@AE1NS Thank you again. Fixed also. If you wish, you can make a pull request for some more bugs you find in future. And I will complete pull with fixes, so you can be in contributors.

roman-rr avatar Jul 19 '23 17:07 roman-rr

Looks good for the moment :)

I really appreciate your work and this awesome support! Thank you very much!

AE1NS avatar Jul 20 '23 05:07 AE1NS

I'm using latest version, 1.3.51, and I just can't call this function. It says '.calcFitHeight is not a function'. I'm using overflow-y to set the scrollable area, but everytime I navigate it breaks, it jjust doesn't calculate the height, therefore no scroll. I wanted to trigger it manually if it's the case, but I can't call this function u guys were talking about. @roman-rr @AE1NS

luckashenri avatar Aug 23 '23 15:08 luckashenri

@luckashenri please create new issue and show your code. I just checked and it works on my side. https://jsfiddle.net/romantonoff/sq2va3xc/

roman-rr avatar Aug 23 '23 16:08 roman-rr