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

Refetch discover feed when changing campuses #81

Merged
merged 1 commit into from
Dec 2, 2019

Conversation

jordanwade
Copy link
Contributor

@jordanwade jordanwade commented Nov 25, 2019

DESCRIPTION

  • Switching campuses should refetch the discover screen.

What does this PR do, or why is it needed?

Adds GET_CONTENT_CHANNELS query to the list of refetchQueries in user locations component. Similar to the work that was done for the Home feed #40

How do I test this PR?

Change your selected campus and click over to the discover screen relevant content should be loaded for your campus.

TODO:

  • PR has a relevant title that will be understandable in a public changelog (ie...non developers)
  • Closes [tag-issues-here]
    Issue on Basecamp
  • No new warnings in tests, in storybook, and in-app
  • Upload GIF(s) of iOS and Android
    iOS - https://v.usetapes.com/7bDWgzSv7d
    Android - https://v.usetapes.com/XI5XEoosOn
  • Set a relevant reviewer

REVIEW:

Manual QA

  • Manual QA on iOS and ensure it looks/behaves as expected
  • Manual QA on Android and ensure it looks/behaves as expected

Code Review: Questions to consider

  • Read through the "Files changed" tab very carefully
  • Edge cases: what assumptions are made about input?
  • What kind of tests could be written?
  • How might we make this easier for someone else to understand?
  • Could the code be simpler?
  • Will the code be easy to modify in the future?
  • What's one part of these changes that makes you excited to merge it?

The purpose of PR Review is to improve the quality of the software.

Copy link
Member

@vinnyjth vinnyjth left a comment

Choose a reason for hiding this comment

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

I like this solution. It's a little inefficient, because it requires us to rerun the GET_CONTENT_CHANNELS query, even if we have never run it in the first place, but unfortunately I don't think there's a clear way to conditionally run that query or simply nuke it from the cache.

@jordanwade
Copy link
Contributor Author

@vinnyjth cool, yeah I wasn't sure how to do it otherwise. I did stumble onto something about InMemoryCache, but again not sure if that would work.
image

@vinnyjth
Copy link
Member

vinnyjth commented Dec 2, 2019

I messed around with it a bit this morning (tried this apollographql/apollo-feature-requests#4 (comment)) but I wasn't able to delete the query itself from the cache, only the data it fetches 😢

@vinnyjth
Copy link
Member

vinnyjth commented Dec 2, 2019

I'm doing one extra test on this, then I will merge

@vinnyjth vinnyjth merged commit 9ace5b8 into master Dec 2, 2019
@vinnyjth vinnyjth deleted the discover-feed-refetch branch December 2, 2019 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants