-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
Travis failed because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks okay, as long as it works
the only thing that's really needed here is that you ought to store the jwt in local storage and load it before the HOC mounts, check what the guy does here: https://github.com/jchavarri/chrome-extension-starter-reason-react/blob/master/src/colorSelector.re
there are bindings for it too
@@ -10,8 +10,7 @@ | |||
"default_popup": "popup.html" | |||
}, | |||
"permissions": [ | |||
"activeTab", | |||
"storage" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was storage removed? are you not storing the jwt in localstorage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah but wouldn't I have to login every time I click on the chrome extension? the two tasks are hand-in-hand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you currently will have to login every time you click on the extension.
this PR only addresses login in and out for the Login
components perspective.
#19 handles login in and out from a broader perspective.
this PR ensures that first step is functional in order to work on the next PRs (#17 #19)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote against holding up the PR until #19 is done.
storing the JWT in storage has nothing to do with the Login
component and its call to the Auth API
@@ -1,13 +1,5 @@ | |||
open Chrome.Extensions; | |||
|
|||
/** Convert value from event to string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this moved from here to services? if anything, it needs to be under a new "utilities.re" file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move it there, in the PR description I said it was extracted because its used across multiple components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in latest commit
src/App.re
Outdated
| JobApp; | ||
|
||
type state = {route}; | ||
type state = {token: string}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option(string) should be used here, but at this point I don't care
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in latest commit
d903c05
to
6638062
Compare
for reference, many people are experiencing the same Coveralls failure: Coveralls GitHub Issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think that the jwt storage & login should be the same PR
for then testing the next part (adding job applications) would become a nightmare
if you want I can approve this but I say we wait for #19 before merging
@@ -10,8 +10,7 @@ | |||
"default_popup": "popup.html" | |||
}, | |||
"permissions": [ | |||
"activeTab", | |||
"storage" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah but wouldn't I have to login every time I click on the chrome extension? the two tasks are hand-in-hand
6638062
to
300f76b
Compare
- Advances #10 by implementing error handling of the login form - Login is now a reducer component. It performs the authentication and via a callback returns the JWT to the App component. - Services has a side-effect method "authenticate" which POSTs the credentials to the Auth backend. If the login is succesful, it returns the JWT to Login If the login fails, it changes the state of Login to DisplayError - Services has a new type "authResponse" to decode the HTTP response - valueFromEvent was refactored to Services because its used in multiple components Closes #16
300f76b
to
d1b4992
Compare
Description
It performs the authentication and via a callback returns the JWT to the App component.
If the login is succesful, it returns the JWT to Login.
If the login fails, it changes the state of Login to DisplayError
Sign in with Google
dummy button forSign Up
,which redirects the user to the web-app sign up page
(when it will be available online)
Closes #16
Checklist