Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[search] Link search results to story screen #37

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

akshaynthakur
Copy link
Collaborator

@akshaynthakur akshaynthakur commented Nov 7, 2023

What's new in this PR

  • Implement fetch_story query that returns a Story object based on story ID
  • Implement routing with params passed between search screen and story screen
  • Updated types of excerpt, content, and story in the schema to be jsonb instead of text

Relevant Links

N/A

Notion Sprint Task

N/A

Online sources

Related PRs

How to review

  • Test that onPress functions work for every search result card, story that's rendered is the story card that was clicked on
  • The story useState can be null, which means that when I'm trying to render parts of it I'm doing a lot of null checking or non-null assertion. Just want to make sure this is ok or use a better method if one exists

Next steps

N/A

Tests Performed, Edge Cases

N/A

Screenshots

screen-20231106-194507.2.mp4

CC: @akshaynthakur

Copy link
Contributor

@surajr10 surajr10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great stuff!

scrollRef.current?.scrollTo({ x: 0, y: 0 });
};

useEffect(() => {
Copy link
Contributor

@surajr10 surajr10 Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

showing a loading spinner before displaying story works on my end!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added an empty dependency array to the useEffect or else I saw an infinite loop of calls to fetchAllStoryPreviews and I think adding the dependency array fits in with what you're desired behavior is.

@@ -77,7 +74,12 @@ function SearchScreen() {
image={item.featured_media}
authorImage={item.author_image}
tags={item.genre_medium}
pressFunction={() => null}
pressFunction={() =>
router.push({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onPress function works on my end for each of the story cards and the story pages display the correct info!


function StoryScreen() {
const [isLoading, setLoading] = useState(true);
const scrollRef = React.useRef<any>(null);
const [story, setStory] = useState<any>();
const [story, setStory] = useState<Story>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one workaround to all the null checks is changing the initial value of the story to be like this: useState({} as Story). You can then remove the ? and ! that are being used without warnings I believe

useEffect(() => {
setLoading(true);
(async () => {
const storyResponse = await fetchStory(parseInt(storyId as string, 10));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another workaround i can think of, is that you can check if the storyResponse that is returned is null, and if so you just display "No story found" in large letters in the center of the screen. theoretically, a user should never see this because the story will exist b/c it had to have for us to get to this screen, but it will at least stop the compiler warnings

@akshaynthakur akshaynthakur merged commit 65602ee into main Nov 8, 2023
2 checks passed
@akshaynthakur akshaynthakur deleted the akshay/expo-params branch November 8, 2023 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants