search

techmatters / terraso-mobile-client

GNU Affero General Public License v3.0
2 stars 0 forks source link

Soil Info sheet - upon selecting "Create site", sheet remains open and user does not see form #1556

Closed DerekCaelin closed 1 week ago

DerekCaelin commented 1 month ago

App Version

142

Account

No response

Platforms

Description

The Soil Info sheet isn't closing when the user selects "create site" and the user doesn't see the form until they manually close the sheet

Steps to Reproduce

  1. tap map
  2. select "learn more"
  3. select "explore the data"
  4. select a soil
  5. scroll to the bottom of the sheet and tap "create site"

Expected behavior

sheet closes and the user sees the "create site" form

Actual behavior

sheet remains open, user needs to manually close it to see the form

Additional context

No response

CourtneyLee333 commented 2 weeks ago

I can repro this on v148.

CourtneyLee333 commented 1 week ago

@knipec I just reviewed this and think we should, instead, remove the Create Site Here button from the soil info sheet.

knipec commented 1 week ago

@CourtneyLee333 Sweet, happy to just remove it :) That's the only place you know of that something navigates to another thing in an overlay sheet, right?

knipec commented 1 week ago

For future engineer reference, this is because overlay sheets are not part of the navigation stack. I think it makes sense to as much as possible treat them as a dead end in navigation flow. However, if we do need to allow that in the future, some approaches Ruxandra and I discussed:

  1. Pass a ref of the overlay sheet to the button, that allows the button to be able to close the overlay sheet when it is pressed (the simplest, preferred)
  2. BottomSheetModalProvider used to be deeper in the component hierarchy than RootNavigator, but was pulled up outside of the RootNavigator to solve some other issue. It may be possible to introduce it further down the tree again, but this seems more complex because wouldn't want to break other things. (Also, there are 2 BottomSheetModalProviders at different levels of the component hierarchy: the outermost one is needed for ListFilters because paper components can open modals; don't recall what the innermost one is needed for.)
  3. Some vague thought that overlay sheets in general could be closed when navigation happens, but that seems least simple.
CourtneyLee333 commented 1 week ago

Currently we have a stack of overlay sheets in this path: Top Soil Matches > open a soil info overlay sheet > tap on InfoButton for Location Score or Soil Properties Score > opens info overlay sheet

It seems to switch one overlay sheet for the other, but the Location Score and Soil Properties Score sheets are currently implemented incorrectly and need to be rebuilt with the proper component. If we need to make overlay sheets a dead end in this case, I think making the soil info sheet a screen instead would probably be best.

knipec commented 1 week ago

Ok, I think that's fine! This is confusing because there are two different libraries at play for concepts that to the user probably both seem like they're about navigation: the react-navigation library, and the gorhom library for bottom sheets / overlay sheets. In the case you mention it's actually just opening another gorhom sheet, not using the react-navigation library (via our useNavigation hook) to navigate sheets. So there still wouldn't be react-navigation stuff coming from the overlay sheet.

paulschreiber commented 1 week ago

Confirmed this is fixed in 159.