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.

Sign up for GitHub

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

Jump to bottom

Pull down to refresh implemented #132

Merged
merged 32 commits into from Jul 31, 2017
Merged

Pull down to refresh implemented #132

Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
f455e30
Merge pull request #1 from gitpoint/master
SammyIsra Jul 17, 2017
9e091a8
Added react-native-cli to avoid onboarding issues in the future
SammyIsra Jul 20, 2017
f52e2f6
Pull down to refresh implemented in profile page
SammyIsra Jul 20, 2017
90c8d76
Pull down to refresh implemented in Repository page. Closes #103.
SammyIsra Jul 20, 2017
8e285b4
Merge branch 'master' into master
SammyIsra Jul 20, 2017
5eb2b51
Fixed some merge issues
SammyIsra Jul 20, 2017
870ea3c
Fixing some more merging issues
SammyIsra Jul 20, 2017
9a86228
Forgot to add myself as a contributor.
SammyIsra Jul 20, 2017
08548e5
Fetching user data and fetching repository data have been moved into …
SammyIsra Jul 21, 2017
e02fe90
Merge branch 'master' into master
SammyIsra Jul 22, 2017
f308a7b
Fixed some merge issues.
SammyIsra Jul 22, 2017
36d8cf7
Renamed the function that fetches the information to be a bit more de…
SammyIsra Jul 22, 2017
5d7736b
Merge branch 'master' into master
SammyIsra Jul 22, 2017
088dc12
Again, fixing my own merge issues. Doing it here with code highlight …
SammyIsra Jul 23, 2017
9d97a3a
Merge branch 'master' into master
SammyIsra Jul 24, 2017
7c11120
Fixed some merge issues, _again_
SammyIsra Jul 24, 2017
f8506e7
Added pull to refresh to the non auth profile screen. Other minor imp…
SammyIsra Jul 24, 2017
438a87f
Fixing indentation inside of `<ScrollView />`
SammyIsra Jul 24, 2017
f7da7a5
Removing extra newline
SammyIsra Jul 24, 2017
899d028
Adding a newline between our components and vendor components.
SammyIsra Jul 24, 2017
a17a572
Added myself as a contributor. Code and Bug Reports
SammyIsra Jul 24, 2017
88eee32
Valid NPM versions need to include three parts (x.y.z)
SammyIsra Jul 27, 2017
15d685b
Removed the double ScrollView. Parallax can now take in refreshContro…
SammyIsra Jul 27, 2017
ec7c9ef
Pull down to refresh now in Issue/PR page (comment list)
SammyIsra Jul 27, 2017
f484cd8
Merge branch 'master' into master
SammyIsra Jul 27, 2017
442690c
Fixed some merging issues and yarn lockfile
SammyIsra Jul 27, 2017
203cc2b
Organizations are now refreshable
SammyIsra Jul 27, 2017
f6ed3ba
Better behavior on the AuthUser pull to refresh
SammyIsra Jul 27, 2017
02c5e67
Even better experience when refreshing AuthProfile. Now we have a fla…
SammyIsra Jul 27, 2017
3e907f9
Refreshing on Issues (specifically) works again
SammyIsra Jul 28, 2017
5d639c1
Merge branch 'master' into master
SammyIsra Jul 28, 2017
66ad3e3
Used '!!' instead of '|| false' as per suggestion
SammyIsra Jul 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/issue/screens/issue.screen.js
View file
Open in desktop
Expand Up @@ -230,7 +230,7 @@ class Issue extends Component {
navigation,
} = this.props;

const isLoadingData = isPendingComments || isPendingIssue;
const isLoadingData = isPendingComments || isPendingIssue || false;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think false is needed here. It will only use that value if the other two values are false.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually having problems on that line. For some reason, I was receiving undefined on both isPendingComments and isPendingIssue and the OR of two undefined is undefined. That would usually not be an issue because of JS's truthsy falsy, but the refreshing prop on <FlatList /> requires a boolean specifically.

Looking over the file again... I am not sure how either of those props can be undefined. They both come from issueReducer and they both have default values.

As a solution to this, I can prepend the comparator with !! to force it to be a boolean, but that would just be avoiding this undefined problem I kinda want to get to the root of.

Copy link
Member

@andrewda andrewda Jul 28, 2017 •
edited

Choose a reason for hiding this comment

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

Huh, weird... !! is probably the best solution for now, but I'm curious what the underlying issue is.

const isLoadingData = !!(isPendingComments || isPendingIssues);

Maybe do something like that for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, I did that on the latest commit. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

Great! This decides crash the application when you open the issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, I don't think I've seen that issue before

const fullComments = !isPendingComments ? [issue, ...comments] : [];
const participantNames = !isPendingComments
? fullComments.map(item => item && item.user && item.user.login)
Expand Down Expand Up @@ -262,8 +262,8 @@ class Issue extends Component {
ref={ref => {
this.commentsList = ref;
}}
onRefresh={this.getIssueInformation}
refreshing={isLoadingData}
onRefresh={this.getIssueInformation}
contentContainerStyle={{ flexGrow: 1 }}
ListHeaderComponent={this.renderHeader}
removeClippedSubviews={false}
Expand Down