shared element transition #75

Nnanyielugo · 2023-01-25T22:31:25Z

No description provided.

…e has not loaded in redux

mjmayank · 2023-01-27T14:51:37Z

src/Screens/Explore/DiveSite.tsx

@@ -248,17 +248,17 @@ const DiveSite: FunctionComponent<DiveSiteProps> = ({ navigation, route }) => {
      <ScrollView
        style={styles.scrollContainer}
        showsVerticalScrollIndicator={false}>
-        <SharedElement id={`item.${navObjectSpot.id}.image`}>
+        <SharedElement id={`item.${(navObjectSpot as Spot).id}.image`}>


rather than re-casting this here all the time, we should re-cast it at the top of the file when it's initialized

doing this is basically just ignoring the type warnings. if navObjectSpot is sometimes optional we need to add logic that ensures that it's fetched/defined

Actually can't typecast when initialised (linter will bleat) because it is an optional navigation param. I can use a question mark for the values that don't need typecasting and typecast the only one that needs it.

Not really. If you check line 227, you'd see where I handled the case where navObjectSpot is undefined. Else navigating to DiveSpot from search would break the app.

I deliberately made it optional because I know not all redirects to that page will pass along the navObjectSpot.

yeah the compiler is correct that this value isn't guaranteed to exist. so we need to create code patterns that ensure it exists, like adding the question mark or checking for null some other way. otherwise for example if the user enters this screen from a deeplink (where they would have just the beach ID and not the navObjectSpot, then it could result in an error/crash)

Yes. Except I purposely told the compiler that that value might not exist. It is reading from a Navigator type object for that page that I defined, and deliberately made optional.

If I change that value and make the compiler thing it will always exist, that error will disappear.

And I've already handled the case for when the object doesn't exist (see line 227 of the same file. mentioned in my previous reply). Deeplink is not the only use case for this. It also happens when the user navigates from explore screen search results. those typically don't carry beach objects with them.

you may be right, but ill need to read the full code more closely when i get time later today.

in general having to cast things this way isn't ideal and there is a better way to do it that still allows the compiler to help catch future bugs, which is why i'm skeptical

just to elaborate, generally the only times you want to cast are when things are at the time something is coming in from an external interface, and you know it's guaranteed to exist otherwise something else has gone wrong (eg an API response with a specific format).

In this case the data could be coming from an external interface (eg deeplink), but the correct typing is still optional as you stated, since that data isn't guaranteed to exist. So then for the rest of the code we shouldn't be doing typecasting and instead working with the compiler to ensure the values exist.

mjmayank · 2023-01-27T14:52:18Z

src/Screens/Explore/DiveSite.tsx

@@ -152,7 +152,7 @@ const DiveSite: FunctionComponent<DiveSiteProps> = ({ navigation, route }) => {

    // just make call to fetch beach if no images, since we're only calling this hook once now
    dispatch(handleFetchDiveSite(currentSpotId));
-  }, [currentSpotId]); // eslint-disable-line react-hooks/exhaustive-deps
+  }, []); // eslint-disable-line react-hooks/exhaustive-deps


why did you change this?

Oh, that was a mistake. Was trying to fix a very strange bug infinity loop bug so I was fiddling with the dependenncy list to try to understand where it came from. Reinstalled now.

mjmayank · 2023-01-27T15:40:02Z

src/Screens/Explore/components/DiveSiteWithoutNavObject.tsx

@@ -0,0 +1,367 @@
+import React from 'react';


i don't think duplicating all this code feels like the correct way to handle this logic. at the very least, we need to them break the whole screen up into smaller components that are imported into dive site with/without nav object, but otherwise it will get really hard to maintain these separate components as we make other design changes

Yes (about breaking the entire screen part). I wasn't sure that the transition would work if the elements it affects are not in the component that it extends with .sharedElements. Maybe I'll try and see what happens.

mjmayank · 2023-01-27T15:40:26Z

src/Screens/Explore/components/DiveSite.tsx

@@ -82,7 +90,8 @@ const DiveSite: FunctionComponent<DiveSiteProps> = props => {
      <TouchableOpacity
        activeOpacity={0.8}
        onPress={() =>
-          props.onPressContainer && props.onPressContainer(props.site.id)
+          props.onPressContainer &&


nit: i think we can delete this since it's already checked above

Tried before. Compiler bleated. Decided to leave it alone. Complier is still bleating.

oh weird. that's fine then

mjmayank · 2023-01-27T15:43:16Z

src/Screens/Explore/DiveSite.tsx

-  }, [currentSpotId, dispatch, reviewInState, diveSiteInState, diveSite]);
+    // just make call to fetch beach if no images, since we're only calling this hook once now
+    dispatch(handleFetchDiveSite(currentSpotId));
+  }, [currentSpotId]); // eslint-disable-line react-hooks/exhaustive-deps


hmm the old dependencies seem more correct imo. although we could probably avoid fetching the nearby sites too frequently. maybe all these fetches should all be in different effects

So I removed it because I figured that function only needs to run once, when the currentSpotId changes. All of the other dependencies will only trigger re-renders. None of those calls need to happen more than once.

the correct dependency array is [currentSpotId, dispatch, diveSiteInState, diveSite] so you can remove reviewInState. I think otherwise the correct change would be to split these up into separate effects. then you can delete the linter comment, and still only have it run the correct number of times for each fetch

initial solution for shared element transition

c5d821a

Nnanyielugo requested a review from mjmayank January 25, 2023 22:31

Nnanyielugo marked this pull request as draft January 25, 2023 22:31

Nnanyielugo added 8 commits January 26, 2023 00:28

add description transition

1b62108

create partial skeleton to denote loading state

f48c93d

reduce api calls in component and show partial skeleton when full sit…

c99d7de

…e has not loaded in redux

remove dispatch as an argument to the effect that handles unsubscribe

b6c2325

display dive site without nav object for search result nav redirects

14c670e

adjust object accessing for Divespot passed as params being optional

e20e6c5

Make shared elements transitions work with explore search

0ccd58a

fix search result bug and make transition to dive site screen

15986b2

Nnanyielugo marked this pull request as ready for review January 27, 2023 12:34

mjmayank reviewed Jan 27, 2023

View reviewed changes

Nnanyielugo added 2 commits January 27, 2023 16:28

replace typecasting with question marks

f2ee086

reinstate dependency item in effect that handles data fetching

1c854dc

mjmayank reviewed Jan 27, 2023

View reviewed changes

Nnanyielugo added 4 commits January 27, 2023 17:06

move dive site with nav object to a separate component

bc7540c

fix weird animation bug on shared search

fb85858

iOS 2.6.5

63cd6e5

fix search and divesite animation issues

9b7f9d3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shared element transition #75

shared element transition #75

Nnanyielugo commented Jan 25, 2023

mjmayank Jan 27, 2023

mjmayank Jan 27, 2023 •

edited

Loading

Nnanyielugo Jan 27, 2023

Nnanyielugo Jan 27, 2023

mjmayank Jan 27, 2023

Nnanyielugo Jan 27, 2023

mjmayank Jan 27, 2023

mjmayank Jan 27, 2023

mjmayank Jan 27, 2023

Nnanyielugo Jan 27, 2023

mjmayank Jan 27, 2023

Nnanyielugo Jan 27, 2023

mjmayank Jan 27, 2023

Nnanyielugo Jan 27, 2023

mjmayank Jan 27, 2023

mjmayank Jan 27, 2023

Nnanyielugo Jan 27, 2023

mjmayank Jan 27, 2023

shared element transition #75

Are you sure you want to change the base?

shared element transition #75

Conversation

Nnanyielugo commented Jan 25, 2023

Choose a reason for hiding this comment

mjmayank Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjmayank Jan 27, 2023 •

edited

Loading