Pull down to refresh implemented #132

SammyIsra · 2017-07-20T05:25:53Z

Resolves #103

Pull down to refresh is implemented in both Profile and Repository pages. The diff seems to be more complicated than what the changes actually are (if there is a way to ignore whitespace on the diff this might make it easier to see).

I simply wrapped the views with a <ScrollView /> with a refreshControl that dictates the behavior of the app when the menu is pulled down.

Because of my onboarding issues, I also added react-native-cli as a dev dependency. This is only needed to avoid a bug. Related issue here

For posterity, here is a demo:

Getting the latest Master with Android support

andrewda

Other than the conflicts, looks good! Thanks!

andrewda · 2017-07-20T05:54:29Z

src/auth/screens/auth-profile.screen.js

+                )}
+              </SectionList>
+            </ParallaxScroll>}
+        </ScrollView>
      </ViewContainer>
    );
  }


This has got quite a few conflicts and is using a pretty old version of the file. For example, most of these SectionList's can be replaced with the <EntityInfo /> component.

andrewda · 2017-07-20T06:02:15Z

src/auth/screens/auth-profile.screen.js

                />
-              </SectionList>}
+              </SectionList>


This email section can be removed as it's included in EntityInfo 😄

Yeah a bit of a mistake when merging... Last commit should have removed it I think, a second swing at merging your master into my branch

Done

andrewda · 2017-07-20T06:19:20Z

Looks good after a quick pass through! I can look at it more tomorrow morning :)

housseindjirdeh

This looks great 🎉 For someone who doesn't have much React Native experience - this is pretty impressive.

Just a few minor nits and I would appreciate if you can upload a gif of the refreshControl in action. Also I remember you mentioned issues with using the precommit hook, are the files you changed formatted with prettier? If the precommit is giving you trouble, try getting a plugin for your IDE (just wondering why all the whitespace is showing)

housseindjirdeh · 2017-07-21T00:35:18Z

.all-contributorsrc

@@ -155,6 +155,15 @@
      "contributions": [
        "code"
      ]
+    },
+    {
+      "login": "SammyIsra",


🎉 🎉 🎉 🎉 🎉

housseindjirdeh · 2017-07-21T00:37:37Z

src/auth/screens/auth-profile.screen.js

@@ -113,71 +114,87 @@ class AuthProfile extends Component {
    }
  };

+  refreshProfile = () => {
+    this.props.getUser();


Can we extract this to a separate method (for example: getUserDetails) and then call getUserDetails in componentDidMount as well as in the ScrollView refreshControl prop?

housseindjirdeh · 2017-07-21T00:39:33Z

src/repository/screens/repository.screen.js

@@ -118,6 +118,14 @@ class Repository extends Component {
    }
  };

+  refreshRepo = () => {
+    const { navigation } = this.props;


Same thing here. Maybe rename this to something like getRepoInfo and call the method within componentDidMount and refreshControl prop so we can avoid duplication

SammyIsra · 2017-07-21T02:48:12Z

@housseindjirdeh @andrewda Thanks for the feedback!
Quick question, where would I upload the gif of the refresh? To the gitter or to the repo?
The files I changed are not formatter with prettier, I am guessing all the whitespace is showing up because I had to increase the indentation of everything by one step, since I was wrapping everything in <ScrollView>.
The other two changes have been implemented and will be pushed shortly!

housseindjirdeh · 2017-07-21T03:50:08Z

@SammyIsra here would be perfect (I don't think gitter even allows uploading attachments 😂). Usually with any PR that I do that has UI changes I always feel a screenshot or gif is super useful

housseindjirdeh · 2017-07-21T03:51:18Z

No worries at all mate looking through everything it looks solid and white space isn't a problem - they'll get formatted when one of use modifies the file thanks to prettier

…their own functions.

SammyIsra · 2017-07-21T04:06:09Z

Hey so, for some reason the repo stopped working on my machine again. I think this is an issue with my machine and not with the repo itself. Would someone be able to make sure this works again? Sorry for the issues caused :/

housseindjirdeh · 2017-07-21T14:07:22Z

what's the issue you're seeing mate, let's try to help in the gitter channel again @SammyIsra

And absolutely I'll take a look at this a bit later when I get the chance

SammyIsra · 2017-07-21T18:14:36Z

@housseindjirdeh sure, I'll post about it on Gitter whenever I get a chance (probably in around 4-ish hours). Hope we can work this out.

SammyIsra · 2017-07-22T06:45:50Z

Gif showing the pull down to refresh on GitPoint! http://i.imgur.com/plWbQX6.gif

SammyIsra · 2017-07-22T06:55:05Z

Fixed some merge issues (but another set of eyes on src/auth/screens/auth-profile.screen.js would be appreciated just in case)

andrewda · 2017-07-22T06:55:29Z

Would it be possible/desired for the stars, forks, bio and owner not to be replace until after being updated? By that I mean keep the current data until the new data is received, then update it so the screen isn't blank for a few seconds.

andrewda · 2017-07-22T06:58:19Z

src/auth/screens/auth-profile.screen.js

+                <UserProfile
+                  type="user"
+                  initialUser={user}
+                  user={user}
                  navigation={navigation}
                />
              )}


Extra parenthesis here, guessing it got messed up in fixing the conflicts (see Travis build).

Yup, merge issues :/ fixed it in the next commit!

…scriptive

SammyIsra · 2017-07-22T17:10:47Z

All change requests have been addressed!

…and VSCode yelling at me is easier than on the Github editor.

housseindjirdeh · 2017-07-27T02:50:56Z

@SammyIsra don't ever rush my friend, take as long as you ever need to <3

Open source should never amount to additional stress, if a feature takes 5 months to complete than so be it :)

SammyIsra · 2017-07-27T02:52:38Z

The ticket itself is limited to User Profile and Repository screens. I've added it to the non logged in User, and the Issue/PR page. Adding a pull down to refresh feature on a page should be easier now! @housseindjirdeh thank you for pointing out the use of ScrollView in react-native-parallax-scroll-view. I've tested the scrolling on my phone and it doesn't seem to overscroll. Do let me know if another issue is found!

SammyIsra · 2017-07-27T03:06:27Z

It seems to work on my phone (Android), and passed all checks :D

andrewda · 2017-07-27T03:34:02Z

One tiny thing I'd change is that (at least on iOS) when on the user auth screen (logged in user) after pulling down to refresh the loading screen is shown again. It works properly on another user's page, refreshing shows the loading spinner in the info area.

andrewda · 2017-07-27T03:34:36Z

Also please add this to organizations!

SammyIsra · 2017-07-27T05:09:42Z

@andrewda I was wondering about that. Looking at the code, the initial "loading" stage in auth user is different than the loading stage of other users. I can certainly take a look!

Gotcha, will add it to Organizations too :)

andrewda · 2017-07-27T06:10:47Z

@SammyIsra Yea I figured that was the case. Would probably be best to fix/change that here because that part's a bit weird as it stands.

…g when we _have_ a profile

SammyIsra · 2017-07-27T07:08:04Z

So I already mentioned this on Gitter but putting this here just in case also. As @andrewda pointed out, when you pull down to refresh on the AuthUser page the entire page goes white while refreshing. The reason for that we don't have an "initial user" to render some of the profile information.

I have two possible solutions for this. First, pass isPendingUser ? {} : user to both user and initialUser to <UserProfile />. The profile still behaves weirdly, but it looks better than a blank screen. You can see what I mean in the render function of auth-profile.screen.json (commit f6ed3ba, Better behavior on the AuthUser pull to refresh)

The second solution is to have a state property called hasInitialUser. The property is set to true whenever an action of type GET_AUTH_USER.SUCCESS is passed to the authReducer (getUser acton creator). This flag indicates that state.auth.user is populated, but not necessarily updated. This flag stays as true while we are getting a new update (we can reference the old user, even temporarily). The only time the flag is set to false is when the user is set to false (commit 02c5e67, Even better experience when refreshing AuthProfile

I kinda prefer the second method since I believe it has a better experience when refreshing. But would still like to get everyone's opinion on it!

SammyIsra · 2017-07-27T18:17:45Z

Hey actually I found a bug on the latest version, so it's not safe yet to commit. The examples I mentioned in the last comment are unaffected tho, so if you want to take a look that part is not related to the bug.

housseindjirdeh · 2017-07-27T23:28:14Z

Thanks @SammyIsra - I'm actually okay with not having pull to refresh for AuthProfile if it turns out to be too much work. Don't think people would want to refresh their own profiles that often to be honest.

Regarding hasInitialUser - still trying to wrap my head around the logic. Is there a reason to have this specific flag if all it does is return true when a user has logged in? Seems like we can just check if the user object exists in AuthReducer no?

SammyIsra · 2017-07-28T03:39:01Z

@housseindjirdeh The flag is set to true not when the user has logged in, but when we have initially fetched the authenticated user. Also, when we log out, we don't empty out the auth.user. Technically the user profile is still there even after logging out. A different option would be to empty out the user when logging out, if that out be better than having a flag.

SammyIsra · 2017-07-28T03:57:28Z

@housseindjirdeh Also, not that much work now that it is already implemented haha. Easy to remove too if we, later on, decide not to have it.

andrewda · 2017-07-28T14:19:50Z

src/issue/screens/issue.screen.js

@@ -230,7 +230,7 @@ class Issue extends Component {
      navigation,
    } = this.props;

-    const isLoadingData = isPendingComments || isPendingIssue;
+    const isLoadingData = isPendingComments || isPendingIssue || false;


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

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.

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?

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

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

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

Temporal fix, I still want to figure out why isPendingComments and isPendingIssue are undefined.

SammyIsra · 2017-07-29T20:59:31Z

I think everything has been addressed now. I can't catch any bugs on the latest commit. Let me know if you see something!

andrewda · 2017-07-29T21:07:44Z

Alright I'll test it out again in an hour or so!

housseindjirdeh · 2017-07-31T18:19:54Z

@SammyIsra this looks good!!!!!! 🎉 🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

andrewda · 2017-07-31T18:25:56Z

🎉

* Added react-native-cli to avoid onboarding issues in the future * Pull down to refresh implemented in profile page * Pull down to refresh implemented in Repository page. Closes #103. * Fixed some merge issues * Fixing some more merging issues * Forgot to add myself as a contributor. * Fetching user data and fetching repository data have been moved into their own functions. * Fixed some merge issues. * Renamed the function that fetches the information to be a bit more descriptive * Again, fixing my own merge issues. Doing it here with code highlight and VSCode yelling at me is easier than on the Github editor. * Fixed some merge issues, _again_ * Added pull to refresh to the non auth profile screen. Other minor improv. * Fixing indentation inside of `<ScrollView />` * Removing extra newline * Adding a newline between our components and vendor components. * Added myself as a contributor. Code and Bug Reports * Valid NPM versions need to include three parts (x.y.z) * Removed the double ScrollView. Parallax can now take in refreshControl. Profile Screen, Repo, and Auth User screen have pull to refresh. * Pull down to refresh now in Issue/PR page (comment list) * Fixed some merging issues and yarn lockfile * Organizations are now refreshable * Better behavior on the AuthUser pull to refresh * Even better experience when refreshing AuthProfile. Now we have a flag when we _have_ a profile * Refreshing on Issues (specifically) works again * Used '!!' instead of '|| false' as per suggestion Temporal fix, I still want to figure out why isPendingComments and isPendingIssue are undefined.

SammyIsra and others added 4 commits July 17, 2017 16:31

Merge pull request #1 from gitpoint/master

f455e30

Getting the latest Master with Android support

Added react-native-cli to avoid onboarding issues in the future

9e091a8

Pull down to refresh implemented in profile page

f52e2f6

Pull down to refresh implemented in Repository page. Closes #103.

90c8d76

andrewda requested changes Jul 20, 2017

View reviewed changes

Merge branch 'master' into master

8e285b4

andrewda previously requested changes Jul 20, 2017

View reviewed changes

SammyIsra and others added 2 commits July 20, 2017 02:08

Fixed some merge issues

5eb2b51

Fixing some more merging issues

870ea3c

Forgot to add myself as a contributor.

9a86228

housseindjirdeh requested changes Jul 21, 2017

View reviewed changes

Fetching user data and fetching repository data have been moved into …

08548e5

…their own functions.

Merge branch 'master' into master

e02fe90

andrewda requested changes Jul 22, 2017

View reviewed changes

SammyIsra added 2 commits July 22, 2017 03:21

Fixed some merge issues.

f308a7b

Renamed the function that fetches the information to be a bit more de…

36d8cf7

…scriptive

SammyIsra and others added 2 commits July 22, 2017 13:39

Merge branch 'master' into master

5d7736b

Again, fixing my own merge issues. Doing it here with code highlight …

088dc12

…and VSCode yelling at me is easier than on the Github editor.

Merge branch 'master' into master

f484cd8

Fixed some merging issues and yarn lockfile

442690c

Organizations are now refreshable

203cc2b

SammyIsra added 2 commits July 27, 2017 02:26

Better behavior on the AuthUser pull to refresh

f6ed3ba

Even better experience when refreshing AuthProfile. Now we have a fla…

02c5e67

…g when we _have_ a profile

alejandronanez approved these changes Jul 28, 2017

View reviewed changes

SammyIsra and others added 2 commits July 27, 2017 23:30

Refreshing on Issues (specifically) works again

3e907f9

Merge branch 'master' into master

5d639c1

SammyIsra mentioned this pull request Jul 28, 2017

Does not load all PR/Issue comments if there are too many #179

Closed

andrewda requested changes Jul 28, 2017

View reviewed changes

Used '!!' instead of '|| false' as per suggestion

66ad3e3

Temporal fix, I still want to figure out why isPendingComments and isPendingIssue are undefined.

housseindjirdeh approved these changes Jul 31, 2017

View reviewed changes

housseindjirdeh merged commit b7fb8f8 into gitpoint:master Jul 31, 2017

Pull down to refresh implemented #132

Pull down to refresh implemented #132

Conversation

SammyIsra commented Jul 20, 2017 • edited

andrewda left a 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

andrewda commented Jul 20, 2017

housseindjirdeh left a 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

SammyIsra commented Jul 21, 2017

housseindjirdeh commented Jul 21, 2017

housseindjirdeh commented Jul 21, 2017

SammyIsra commented Jul 21, 2017

housseindjirdeh commented Jul 21, 2017

SammyIsra commented Jul 21, 2017

SammyIsra commented Jul 22, 2017 • edited

SammyIsra commented Jul 22, 2017

andrewda commented Jul 22, 2017

andrewda Jul 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SammyIsra commented Jul 22, 2017

housseindjirdeh commented Jul 27, 2017

SammyIsra commented Jul 27, 2017

SammyIsra commented Jul 27, 2017

andrewda commented Jul 27, 2017

andrewda commented Jul 27, 2017

SammyIsra commented Jul 27, 2017

andrewda commented Jul 27, 2017

SammyIsra commented Jul 27, 2017

SammyIsra commented Jul 27, 2017

housseindjirdeh commented Jul 27, 2017

SammyIsra commented Jul 28, 2017 • edited

SammyIsra commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewda Jul 28, 2017 • edited

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

SammyIsra commented Jul 29, 2017

andrewda commented Jul 29, 2017

housseindjirdeh commented Jul 31, 2017

andrewda commented Jul 31, 2017

SammyIsra commented Jul 20, 2017 •

edited

SammyIsra commented Jul 22, 2017 •

edited

andrewda Jul 22, 2017 •

edited

SammyIsra commented Jul 28, 2017 •

edited

andrewda Jul 28, 2017 •

edited