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

[BUG] calcFitHeight calculates wrong value #192

Closed
AE1NS opened this issue Sep 6, 2022 · 23 comments
Closed

[BUG] calcFitHeight calculates wrong value #192

AE1NS opened this issue Sep 6, 2022 · 23 comments
Assignees
Labels
bug Something isn't working

Comments

@AE1NS
Copy link

AE1NS commented Sep 6, 2022

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.

@roman-rr roman-rr self-assigned this Sep 6, 2022
@roman-rr roman-rr added the bug Something isn't working label Sep 6, 2022
@AE1NS
Copy link
Author

AE1NS commented Sep 7, 2022

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.

@roman-rr
Copy link
Collaborator

roman-rr commented Sep 7, 2022

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

@roman-rr
Copy link
Collaborator

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

@AE1NS
Copy link
Author

AE1NS commented Sep 12, 2022

Just tested it. Looks good, thank you!

@AE1NS AE1NS closed this as completed Sep 12, 2022
@AE1NS
Copy link
Author

AE1NS commented Sep 13, 2022

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/

@roman-rr
Copy link
Collaborator

@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.

@AE1NS AE1NS closed this as completed Sep 28, 2022
@AE1NS
Copy link
Author

AE1NS commented Jun 29, 2023

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 AE1NS reopened this Jun 29, 2023
@roman-rr
Copy link
Collaborator

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

@AE1NS
Copy link
Author

AE1NS commented Jun 30, 2023

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.

@roman-rr
Copy link
Collaborator

@AE1NS Ok I'll re-check.

@AE1NS
Copy link
Author

AE1NS commented Jul 6, 2023

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.

@roman-rr
Copy link
Collaborator

roman-rr commented Jul 9, 2023

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

@AE1NS
Copy link
Author

AE1NS commented Jul 10, 2023

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).

@roman-rr
Copy link
Collaborator

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

@roman-rr
Copy link
Collaborator

@AE1NS Please check.

@AE1NS
Copy link
Author

AE1NS commented Jul 12, 2023

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.

@roman-rr
Copy link
Collaborator

roman-rr commented Jul 16, 2023

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

@AE1NS
Copy link
Author

AE1NS commented Jul 18, 2023

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
Copy link
Author

AE1NS commented Jul 18, 2023

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.

@roman-rr
Copy link
Collaborator

@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.

@AE1NS
Copy link
Author

AE1NS commented Jul 20, 2023

Looks good for the moment :)

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

@AE1NS AE1NS closed this as completed Jul 20, 2023
@luckashenri
Copy link

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

@roman-rr
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants