Temzasse / react-modal-sheet

Flexible bottom sheet component built with Framer Motion to provide buttery smooth UX while keeping accessibility in mind 🪐
https://temzasse.github.io/react-modal-sheet/
MIT License
828 stars 78 forks source link

Allow sheet to be height of content of the sheet #83

Closed nickgraffis closed 1 year ago

nickgraffis commented 2 years ago

This seems to help with #69 however notably it does not dynamically update the sheet height.

You can click the box to create more boxes if you look at the example added. This won't make the sheet bigger until it is closed and reopened.

Temzasse commented 2 years ago

I wonder how should this work in conjunction with snapPoints 🤔

Now if you set snap points eg. to snapPoints={[0.9, 0.5, 0.1]} in your example it will allow you to drag the sheet beyond the content height which will cause UI issues:

Screenshot 2022-07-24 at 23 00 49
Temzasse commented 2 years ago

I wonder how should this work in conjunction with snapPoints 🤔

Now if you set snap points eg. to snapPoints={[0.9, 0.5, 0.1]} in your example it will allow you to drag the sheet beyond the content height which will cause UI issues:

Screenshot 2022-07-24 at 23 00 49

One solution could be to disallow using both sheetHeight="content" and snapPoints at the same time. I have hard time formulating how should those two even work together if we were to allow using them both at the same time 🤔

nickgraffis commented 2 years ago

Possibly for the snapPoints issue we could just have the content height property take precidence. Let me toy with it, but thinking that if you set both snapPoints and sheetHeight to content-height that the content height would take precidence.

So if the content height was less than the height that the chosen snapPoint would need, the content height would be used.

I'll look into both of these.

Thanks!

Temzasse commented 2 years ago

Possibly for the snapPoints issue we could just have the content height property take precidence. Let me toy with it, but thinking that if you set both snapPoints and sheetHeight to content-height that the content height would take precidence.

So if the content height was less than the height that the chosen snapPoint would need, the content height would be used.

I'll look into both of these.

Thanks!

This sounds like a viable solution 👍

I played around with measuring the content height with react-use-measure to get the actual height value in JS. Didn't have enough time to get anything proper working so let's see what you come up with 😄 If you end up using that lib too then one thing that I had to change was that the sheet container animate needs to be delayed one render cycle so that we are able to measure the content height before showing the sheet.

Temzasse commented 1 year ago

@nickgraffis have you by any chance had the time to work on this? 😊

Temzasse commented 1 year ago

I have worked on a potential implementation but I need to test it a bit more before I can release it. I leaning towards not supporting snapPoints when the sheet height is set to follow the content height. I'm also considering using the detent naming for the prop even though it is not very common term in English - I like how it matches with the Apple bottom sheet terminology.

nickgraffis commented 1 year ago

😂 I have not had a chance - but this was a good ping as a reminder. I see your notes above. I'll take swing this weekend

Temzasse commented 1 year ago

@nickgraffis I hope you have had a great weekend! I had some time to work on this feature so here's my suggestion for the implementation: https://github.com/Temzasse/react-modal-sheet/pull/96

If you haven't gotten too far with your implementation would you be okay with taking a look at the PR and give feedback? 🙂

If everything looks okay I can merge and release it during next week.

nickgraffis commented 1 year ago

Closing because of #96