-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[MAGE] GH login + so many aMAGEing updates #1546
Conversation
Hello from my test account 👋 It seems that the method for checking if a user starred the repo doesn't work 😢 |
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 job @vincanger !
I think all is pretty solid, and nice job on doing all these changes! I left some comments on how to improve things, check them out.
Oh wow, why not? |
I addressed all your comments and tried to clean up and extract as many things as I could. I think the header works a lot nicer now that it accepts children and the StatusPill as a dependency. To address the GH api issue, we need to send the API the user's access token which we receive on successful login to GH via OAuth. Unfortunately, Wasp doesn't expose this token in any way for us to use, so that is out of the question. |
Oh that is interesting. Is that a missing feature in Wasp? Sounds like it is something we should add? @infomiho you are the expert here, what are your thoughts? Should we create an GH issue for it, if we don't already have one? |
ok @Martinsos, I added the return declaration to the UserPage, pushed the changes, and addressed all remaining comments. |
Yes, we are not giving the developer access to the Github Access Token in the It should probably be an issue, but I wouldn't add it "hey we need to support giving the access token to the user" I would do it inside of the overall auth effort of moving to https://github.com/wasp-lang/openid-client-experiment and see how we do it. |
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.
@vincanger I did another round of reviewing!
ok @Martinsos -- reviewd and updated. awaiting your final review :) |
@Martinsos updated! I feel pretty confident that the naming of names was done in the name of clean code :) |
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.
@vincanger nice job with cleaning up the names -> we are not yet there completely I think, but you brought them close enough so I could understand what they are, which is already great! I left some suggestions on how to possibly improve them further + a few other small comments and one possible mistake.
Please go through all of these, also resolve any comments where my response was last and you are happy with it, and then I will take another look!
ok @Martinsos ! the renaming of renamed names is complete :) |
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.
@vincanger nice job, I think this is it!
I left a couple small comments, please give them a look and handle them as you wish, and then you can merge! I approved the PR.
Description
Select what type of change this PR introduces: