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

Add a store for cloneable repositories #6278

Merged
merged 50 commits into from
Dec 10, 2018
Merged

Conversation

niik
Copy link
Member

@niik niik commented Nov 23, 2018

Overview

As part of the onboarding work I'm doing in #5686 I'm going to need access to the list of "cloneable" repositories in the blank slate. Currently the logic for fetching repositories available to the user from the API is contained within the clone dialog itself. This PR extracts that logic into a separate store responsible for loading and refreshing the list of repositories.

The improvements in this PR made the disadvantages of our current shared state model all the more noticeable (you couldn't switch between tabs and maintain selection) so I took the opportunity to remove the shared state n favor of per-tab as users would expect (see #5937). Each tab now maintains its own selection, filter text, path, etc.

Closes #5937 by maintaining separate state for each of the tabs in the clone dialog.

Description

Additional benefits of this change

  • Since the repositories available for a user is now persisted we no longer have to load the repository list on each tab switch inside the Clone dialog.
  • Users will no longer have to wait for the repository list to be loaded every time they open the dialog and can instead choose to refresh manually with a new button

image

Memoization

This PR introduces a memoization pattern that we haven't used before in the app which I think could use some further explanation. See the react blog for a good write-up but basically I've made the CloneGithubRepository component pure and stateless and used memoization to ensure that we only recompute the filter list groups when necessary.

Release notes

[Improved] Clone dialog remembers selections, filter text, and path in each tab
[Improved] The list of repositories in the clone dialog are no longer loaded from the API every time the dialog is opened or when the selected tab changes.

TODO

  • More documentation
  • Automatically refresh the repository list when some amount of time has elapsed since last time?
  • See if we can't get rid of the null state for repositories in the CloneGitHubRepository component
    • Punting on these two for now, the PR is big enough as it is.

@billygriffin
Copy link
Contributor

billygriffin commented Nov 29, 2018

Are there some release notes related to #5937 that we could/should include here? Maybe something like:

[Improved] Maintains state on tabs for different methods of cloning repositories

Thoughts?

@niik
Copy link
Member Author

niik commented Dec 3, 2018

@iAmWillShepherd Sorry for the post-review update but I found a race condition that I had missed when the accounts store was updating at the same time as the apiRepositoriesStore was so I decided it was better to fix it now before this PR lands.

iAmWillShepherd
iAmWillShepherd previously approved these changes Dec 3, 2018
Copy link
Contributor

@iAmWillShepherd iAmWillShepherd left a comment

Choose a reason for hiding this comment

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

Looks good. Appreciate your documentation is so clear and concise 👍

@billygriffin billygriffin modified the milestones: 1.6.0, 1.5.1 Dec 10, 2018
@niik
Copy link
Member Author

niik commented Dec 10, 2018

Ready for one last lookie-loo!

@billygriffin
Copy link
Contributor

Shifting this to 1.5.1 per our team sync today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants