refactor(components-native): Refactor ContentOverlay for Behavior Clarity

[ZakaryH]

Motivations

it's very hard to know what the expected behaviour of ContentOverlay is for 3 aspects:

• height
• draggable
• show dismiss

these are intertwined in very non obvious ways, overriding and affecting each other. for example there's a line in the snapPoint logic that checks if ContentOverlay is draggable. draggable is affected by a few props, and notably onBeforeExit can silently stomp on any explicitly provided value for isDraggable.

 isDraggable = onBeforeExit ? false : isDraggable;
...
    if (fullScreen || !isDraggable || adjustToContentHeight) {
      return undefined;
    }

all that is to say it's too complicated to track, and that impacts our ability to have confidence that when we replace the underlying library, that we have done it in a way that is more or less a 1:1 with the existing behaviour because we don't know what that even is.

to help facilitate this change that we have in another branch, I'd like to refactor this into a smaller, testable, self documenting module that returns abstracted, meaningfully named behaviors like initialHeight: "fullscreen" so that we know what to do, but we're not tied to the underlying library and how it would precisely make the ContentOverlay "fullscreen".

Changes

Added

a new, pure function computeContentOverlayBehaviour that figures out the height, draggable, and dismiss show behavior given all the various variables

added tests for this function to assert what we expect to be returned from the function

Changed

moved the logic for these 3 concepts out of the component and into the function, using it instead

Deprecated

Removed

Fixed

Security

Testing

Changes can be
tested via Pre-release

---

In Atlantis we use Github's built in pull request reviews.

[jdeichert]

This is a great improvement! I know you have a conflict and need to update this PR, so I'll re-review after that.

As you're merging down to the base branch, I'll skip QA here and do it all again on the base PR when that's ready for another pass. I read the code thoroughly and pretty sure all of the cases match what is expected 👍

[cloudflare-workers-and-pages]

Deploying atlantis with    Cloudflare Pages

Latest commit:	5374bd5
Status:	✅  Deploy successful!
Preview URL:	https://a2d7fa5b.atlantis.pages.dev
Branch Preview URL:	https://improve-content-overlay-test.atlantis.pages.dev

View logs

[ZakaryH]

updated this after learning a bit more about the onChange method. see comment below.

[ZakaryH]

position is a bit of a fickle arg to use

it's always going to be a value that is relative to the 100% snap point, which I find a bit hard to reason about.

if you have a 100% snap point, when it's at that snap point position is 0
if you have some other snap point less than that, position is some number - let's say 98 which is in pixels
if you are closed, then position is a very big number like 762

so this does work while we have a 100% snap point - but it won't work if that ever changes and I find it just harder to think about. that's not to say the index is perfect either, it has some quirks with the snap points and dynamic entries - but I find that less subject to really arbitrary pixel values that totally depend on the screen size.

eg. 762 could be closed, but it could also just be a small snap point on a larger screen. we can't know from just that number. so effectively, we can only say anything definitive about the 0 value

[jdeichert]

So I understand that onChange from BottomSheetModal gives us index and type but that seems to be undocumented on their website...

I'm confused about this line. In your screenshots, I see index being 0, -1, 1 and here you have index >= snapPoints.length. I assume 1 = open, but how could it ever be more than snapPoints.length ? And what happens if we have 2 snapPoints in the future, would we still receive index=1 meaning it's open?

Or maybe I'm confused and index actually maps to a value in our snapPoints array... but it's not zero-indexed? Something like this snapPoints[index - 1]? 🤔

[ZakaryH]

yeah I had to dig into the types and source code. the docs only say it provides an index :/

so the way the index can be more than snapPoints.length is when you have enableDynamicSizing

with our current setup, we have exactly 1 snap point that we provide explicitly: ["100%"]

however, with enableDynamicSizing what it does is it adds another entry, but internally. it won't modify our snapPoints array which is fair.

since our snap point is 100%, I would assume that's the maximum. I can't imagine the dynamically generated snap point would ever be greater than that, even if we had too much content to fit. this means that when we aren't at that full capacity scenario we'll likely end up with something like: [400, "100%"]

so, we would have 3 possible indices: -1 (closed), 0 (400 pixels dynamically added) and 1 (100% from us)

if I made my condition index == snapPoints.length which is 1 when we change to the largest index of 1..... wait you're right. that would work wouldn't it?

it's actually the non enableDynamicSizing case that might fail. because our index would be 0 and our snapPoints.length would be 1. I have it backwards!