-
Notifications
You must be signed in to change notification settings - Fork 793
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
Navigate to auth profile screen for the current user #211
Conversation
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.
🎉 Awesome!
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.
One remark which i think should be addressed. 👍 for fixing this!
})} | ||
onPress={() => { | ||
navigation.navigate( | ||
authUser.login === item.login ? 'AuthProfile' : 'Profile', |
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 noticed that you use the short if syntax on 1 line here and on line 255 for example of the 'comment-list-item.component.js' file you spread it over multiple lines. Would be nice if we have a consistency.
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.
@RolfKoenders yep, it's good that you noticed this! I thought about this, I also did not want to duplicate the code, but where to put it? In utils
? And how to call the function correctly, something like getProfileScreen
?
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'm okay with leaving the logic here, I don't think this is a helper we would essentially need in utils
.
Regarding @RolfKoenders's comment, you're right but this is something prettier
and our precommit hook should take care of actually. @lex111 if I'm not mistaken this is going through the hook and getting prettified correct?
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.
@housseindjirdeh yep, my hooks are working, I'm watching this.
Good stuff thank you @lex111 :) |
Now the screen auth profile can only be when navigating from the tabbar, although it is displayed for the current user from other places: if the user is on the organization list, if it is mentioned in the issue description or in its comments. Of all these places user should be navigated not to the normal profile screen, but to the auth profile.
From this follows such a problem that I can follow myself (:rofl:), or again, organizations will not all be displayed.