digidem / comapeo-mobile

The next version of Mapeo mobile
GNU General Public License v3.0
5 stars 0 forks source link

fix: fix visual issues with shared content component used in bottom sheet modal #415

Closed achou11 closed 2 weeks ago

achou11 commented 4 weeks ago

changes to the original implementation from Mapeo Mobile (which did not have many of the issues) when porting it to here led to a few visual issues when trying to use the BottomSheetContent component:

the original intent of this content component was to unify the visual and functional behavior of the content inside of the bottom sheet modals, which is designed very intentionally.

this PR attempts to update screens where using the shared BottomSheetContext makes sense (which is most of them). the current exception is the GPSPermissionsModal, which is not using the shared BottomSheetModal component so would require some more work to convert (can be done as a follow-up).

also fixes an issue where we were using a context-based hook without the provider being set up (react-native-safe-area)

important implementation notes:


ErikSin commented 3 weeks ago

Is this desired design (with the space where the description belongs)? It feels a little akward to me, it seems better to get rid of that space an dynamically size the modal (Which I think we were doing before)

image
achou11 commented 3 weeks ago

Is this desired design (with the space where the description belongs)? It feels a little akward to me, it seems better to get rid of that space an dynamically size the modal (Which I think we were doing before)

yeah this is an unfortunate consequence of adding a minimum height for the sheet content in the case of the non-fullscreen variant. without it, the re-introduced flex behavior that fixes the fullscreen variant of the content doesn't play well with the bottom sheet modal dep's dynamic sizing behavior 😓

basically, super tedious to figure out! i have a couple of quick ideas i can experiment with, but may punt on this and instead just fix the cases where the fullscreen variant is broken...

achou11 commented 2 weeks ago

Instead of passing the full screen prop to both the Modal and the content could we not just use a context? We know that the content will always be wrapped by the modal (and if is not, it should throw an error). I think having a full screen prop in two places is not the best design choice.

can give it a try! Agree that having to remember to specify the prop in two separate places isn't ideal

achou11 commented 2 weeks ago

@ErikSin good suggestion with the context! see https://github.com/digidem/comapeo-mobile/pull/415/commits/b8389d5548a685c2b91f0e5267231d00defca184 for changes, but to summarize: