feat(auth): use WebView for authentication #323

machour · 2017-09-11T12:18:57Z

Closes #109 and #221

Rather than using another library to handle Android, I went ahead and used the WebView component from ReactNative. This seems to be working fine for me on iOS Simulator and my Android Device.

I would really love to have a thorough review of my code, as I feel I've kinda abused the view system here. (and it's the app entry point, so it's a really critical change 😆 )

lex111

Really nice work 👍

machour · 2017-09-11T15:22:18Z

Thanks! Although it still seems buggy on my device, I sometime find myself logged in with an empty events tab and nothing clickable. So still WIP.

housseindjirdeh

First and foremost, this is such a great job @machour it's amazing 😍 You've made it feel like a separate window isn't even opening by rendering a WebView with a section underneath:

I know this is a WIP progress but here's a few things I noticed. Noticing an issue on the simulator however with the navigator consistently firing and navigating to Main and I'm not sure why 🤔

When I reload the app, I notice just like you mentioned: a logged in events tab with nothing clickable and showing.

Another thing is the Cancel button you've included (which I think is just awesome :)). Unfortunately if I try to cancel while the loading step is firing it doesn't necessarily stop the call.

Notice how the original Welcome screen remains in the back as well? I love the new Preparing GitPoint screen you have so we'll have to:

Make sure the cancel button isn't there (or disabled) when the call is being fired since the user can't necessarily cancel the call
Remove the Welcome Screen entirely since we have this new intermediate step

OR you can just make sure that clicking sign in will navigate directly to the Welcome Screen and not rely on the webview anymore (happy to have it restyled to match your intermediate Preparing GitPoint screen.

housseindjirdeh · 2017-09-11T16:00:19Z

src/auth/screens/login.screen.js

+    height: '100%',
+    width: '100%',
+    textAlign: 'center',
+    backgroundColor: '#1f2327',


Let's put #1f2327 in the colors file. Can call it whatever you think sounds reasonable (githubDark or something if you like).

machour · 2017-09-13T15:55:03Z

@housseindjirdeh I think I fixed the problem with iOS by using Linking only for Android.
I also disabled the cancel button when needed, revamped the Welcome screen to be consistant and cleaned up my UI mess.
Could you give it a new try?

lex111 · 2017-09-13T19:06:53Z

src/auth/screens/login.screen.js

+  onLoadEnd = e => {
+    const url = e.nativeEvent.url;
+
+    if (url === 'https://github.com/session') {


I'm just thinking out loud: can made a separate method, because this code line two times is used, then the meaning of this check will be more clear?

Great advice, thanks! Refactored here: c2c4d56

machour · 2017-09-13T23:49:57Z

I went ahead and fixed #182 in this same PR by resetting the WebView's cookie as per @andrewda recommendation. The logout is now instantaneous (maybe even too quick 💭 )

I used https://github.com/joeferraro/react-native-cookies to clear the cookies, which needs some extra step in XCode and Android build, documented in their README. I didn't commit the ios/ and android/directories cause I had to tweak a lot of weird things to have a working environment in the same place. I'd rather have one of you guys with a cleaner environment do it when merging this PR.

andrewda · 2017-09-14T05:23:22Z

src/locale/languages/en.js

@@ -4,6 +4,9 @@
 export const en = {
  auth: {
    login: {
+      connectingToGitHub: 'Connecting to GitHub..',
+      preparingGitPoint: 'Preparing GitPoint..',


Add another period to these: Connecting to GitHub... and Preparing GitPoint....

@machour and more you need to add new keys to other languages, otherwise there will be errors.

@andrewda thanks, fixed!
@lex111 currently if a string is missing, it's displayed with an error Missing translation for tr.auth.login.cancel. Isn't that good enough while working on master? Or should I introduce the keys in all language with the english sentence?

@machour yes, you need to add English sentences to all languages, just so that there are no such errors.

@lex111 done buddy! I think I'll tackle down i18n right after auth & markdown 😄

lex111 · 2017-09-14T12:53:43Z

src/locale/languages/fr.js

@@ -2,6 +2,9 @@
 export const fr = {
  auth: {
    login: {
+      connectingToGitHub: 'Connexion à GitHub..',
+      preparingGitPoint: 'Configuration de GitPoint..',


Missing dot 😄

not in french ;)

Wow, I did not know 😃

lex111 · 2017-09-16T17:28:00Z

src/auth/screens/login.screen.js

@@ -236,6 +236,7 @@ class Login extends Component {
          <View style={styles.container}>
            <Modal
              animationType="slide"
+              onRequestClose={() => null}


machour · 2017-09-16T20:03:20Z

ios/GitPoint.xcodeproj/project.pbxproj

@@ -623,6 +642,7 @@
 			isa = PBXGroup;
 			children = (
 				273F19201F1E1AD5004B4410 /* libCodePush.a */,
+				48F9DE9B1F6DB8C60064194C /* libCodePush.a */,


@housseindjirdeh not sure why Xcode added this line, you may want to have a closer look

machour · 2017-09-16T20:03:34Z

ios/GitPoint.xcodeproj/project.pbxproj

@@ -1148,6 +1181,20 @@
 			remoteRef = 48F49F661F19026D0012FAD6 /* PBXContainerItemProxy */;
 			sourceTree = BUILT_PRODUCTS_DIR;
 		};
+		48F9DE9B1F6DB8C60064194C /* libCodePush.a */ = {


same here @housseindjirdeh

machour · 2017-09-16T20:06:18Z

@housseindjirdeh @andrewda @lex111
I just cleaned my project and was able to include links in android/ and ios/ for react-native-cookies. I thing this PR is all set now.

I'd love to see this merged soon in order for it to be fully tested by everyone before making it to the next release 🙏

housseindjirdeh

This works beautifully mate 😍

Signing out NOW works perfectly when tested on my simulator. Had this error however:

But cleaning and rebuilding in XCode fixed things 👍

machour added 3 commits September 11, 2017 12:33

feat(auth): Use RN WebView for auth

2481989

Merge remote-tracking branch 'upstream/master' into webview-for-auth

90d5ba6

fix(auth): Make this work for Android device

17bc901

lex111 approved these changes Sep 11, 2017

View reviewed changes

housseindjirdeh requested changes Sep 11, 2017

View reviewed changes

machour added 4 commits September 13, 2017 15:02

Merge remote-tracking branch 'upstream/master' into webview-for-auth

1a3e743

fix(ui): Use Webview.renderLoading() instead of nasty zIndexing

bb209e9

fix(auth): Use Linking only for Android. Clean up the UI

75458e2

feat(ux): Toggle cancel button state, redesign welcome screen

2cac478

andrewda changed the title ~~[wip] Webview for auth~~ wip: feat(auth): use WebView for authentication Sep 13, 2017

lex111 reviewed Sep 13, 2017

View reviewed changes

machour changed the title ~~wip: feat(auth): use WebView for authentication~~ feat(auth): use WebView for authentication Sep 13, 2017

machour added the ready for review label Sep 13, 2017

machour mentioned this pull request Sep 13, 2017

Strange behavior after logout #182

Closed

machour added 2 commits September 14, 2017 00:37

feat(auth): Avoid logout Webview by clearing cookies

30bed12

refactor(auth): use a single toggle function for cancel button

c2c4d56

andrewda requested changes Sep 14, 2017

View reviewed changes

machour added 3 commits September 14, 2017 13:05

fix(merge): Fix merge conflict

aeb5edf

fix(i18n): Fixed english grammar and translated into french

920dcfc

feat(i18n): Add new localized strings in other languages

ca6daca

lex111 reviewed Sep 14, 2017

View reviewed changes

fix(modal): Silence warning

9d46019

lex111 reviewed Sep 16, 2017

View reviewed changes

fix(auth): Add Android and iOS linking

267565d

machour commented Sep 16, 2017

View reviewed changes

machour added 2 commits September 16, 2017 21:07

Merge remote-tracking branch 'upstream/master' into webview-for-auth

6a7c24d

chore(merge): Merge from master

50074c9

machour requested review from andrewda and housseindjirdeh September 23, 2017 18:48

housseindjirdeh approved these changes Sep 23, 2017

View reviewed changes

housseindjirdeh merged commit 0608c9d into gitpoint:master Sep 23, 2017

machour mentioned this pull request Sep 23, 2017

Implement use of the in-app web view on Android #221

Closed

chinesedfan mentioned this pull request Nov 9, 2019

Login problem since RN 0.59 #913

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): use WebView for authentication #323

feat(auth): use WebView for authentication #323

machour commented Sep 11, 2017

lex111 left a comment

machour commented Sep 11, 2017

housseindjirdeh left a comment •

edited

Loading

housseindjirdeh Sep 11, 2017

machour commented Sep 13, 2017

lex111 Sep 13, 2017

machour Sep 13, 2017

machour commented Sep 13, 2017

andrewda Sep 14, 2017 •

edited

Loading

lex111 Sep 14, 2017

machour Sep 14, 2017

lex111 Sep 14, 2017

machour Sep 14, 2017

lex111 Sep 14, 2017

machour Sep 14, 2017

lex111 Sep 14, 2017

lex111 Sep 16, 2017

machour Sep 16, 2017

machour Sep 16, 2017

machour commented Sep 16, 2017

housseindjirdeh left a comment

feat(auth): use WebView for authentication #323

feat(auth): use WebView for authentication #323

Conversation

machour commented Sep 11, 2017

lex111 left a comment

Choose a reason for hiding this comment

machour commented Sep 11, 2017

housseindjirdeh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machour commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machour commented Sep 13, 2017

andrewda Sep 14, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

machour commented Sep 16, 2017

housseindjirdeh left a comment

Choose a reason for hiding this comment

housseindjirdeh left a comment •

edited

Loading

andrewda Sep 14, 2017 •

edited

Loading