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

Simple test login implementation #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

alexedev
Copy link
Collaborator

@alexedev alexedev commented Aug 1, 2017

after login mutation server response the token is stored in localStorage and then sent in Authorization header on each request. the currentUser query in Login.js is called on first render but after mutation there is no auto re-render so for now I used window.location.reload() in the mutation callback (right after storing the token)

meanwhile will try to figure out why the re-render is not triggered

@alexedev alexedev requested a review from michalsanger August 1, 2017 08:38
@alexedev alexedev force-pushed the 4-login-mutation branch 2 times, most recently from 6c1b9c9 to eb1ecae Compare August 1, 2017 09:19
@alexedev alexedev requested a review from fallion August 1, 2017 09:19
@michalsanger
Copy link
Owner

Plz setup Prettier to use single quotes

@alexedev
Copy link
Collaborator Author

alexedev commented Aug 1, 2017

@michalsanger done

localStorage.setItem('token', response.login.token);
}
if (window) {
window.location.reload();
Copy link
Owner

Choose a reason for hiding this comment

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

Not happy with this hack. Relay should take care of passing new props to components so re-render should happen. Isn't is a sign of bad implementation fi we need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes sure, it is a hack. had no idea how to make it re-render (I mentioned it in the first comment here)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as I understood from our short offline conversation at the beginning of sprint planning, you already tried to fix it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if not, then I will try to update store with updater and trigger re-rendering from the changes

Copy link
Owner

Choose a reason for hiding this comment

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

Check it here 3bf63a7

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks nice. pulled from that branch and committed it here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants