oncode / handorgel

Accessible W3C conform accordion written in ES6.
https://oncode.github.io/handorgel/
MIT License
261 stars 25 forks source link

Nesting - height: auto #12

Closed iskrisis closed 4 years ago

iskrisis commented 5 years ago

Hi! First thank you for your work! I am trying to use handorgel inside another handorgel. It actually does not even have to be handorgel just anything that changes height.

The issue is that obviously the height of content is fixed px number. I understand this is needed for the animation back. The solution probably would be to have middlestep with height: auto. I am wondering if you've given this any thought - maybe its a bad idea.

var accordion = new handorgel(el, {});
accordion.on('fold:opened', (fold) => {
    fold.content.style.height = 'auto';
});

accordion.on('fold:close', (fold) => {  
    fold.content.style.height = fold.content.offsetHeight + 'px';
});

This naive solution works for the nested handorgel the parent handrogel.content is height: auto so it works nicely. The problem si closing - first the event seems to fire after the resize so it animates instantly it goes from height: auto to height: 0px. Then there are things like the resize that has to also use 'auto'. But i am just wondering if there is some reason why you didn't go for this kind of implementation? Is it better to solve nesting by using events and recalculate parent handorgel accordigly (this approach seemed to work even worse for me)?

oncode commented 5 years ago

Hi @iskrisis I think you are right, the height should be set to auto, when the animation has finished and stay like this to avoid calculations during resize. I hope that I can fix it next weekend.

iskrisis commented 5 years ago

I am sorry i wasn't able to do pull request i am in middle of a job and i just had to bang out nested animated accordion quickly. I ended up doing it by hand using velocity.js. But since i tried like every accordion there is i can safely tell you yours is the best out there right now. I will for sure use it in the future. I am not sure why but there is big collapsible/accordion drought everything is either unmaitained/legacy that animates slowly using JS or made for react/angular/vue.

If your aim is popular library i think you are on right track! Also the codebase is nice i digged through it when i wanted to make my changes and if i understood it then anyone can! In the end i had some problems with build process so i gave up.

Thank you for your work.

qzminski commented 4 years ago

Hey @oncode, first of all thanks for the great library! Do you have any plans on fixing this issue? @iskrisis solution seems to work but is not perfect – the content collapses immediately without any animation, which should likely not happen.

oncode commented 4 years ago

Hi @qzminski, will look into it ASAP. Thanks for reminding!

oncode commented 4 years ago

Hey @qzminski @iskrisis Finally I looked into it and the height is set now to auto after animating or directly to auto if no transition is desired. So I could remove the resizing listener, methods and no-transition classes, because they are not needed anymore. It's published as 0.5.0 version.

qzminski commented 4 years ago

Works like a charm, thanks a lot!