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

Show log in github from FE #4979

Merged
merged 46 commits into from
Aug 28, 2024
Merged

Show log in github from FE #4979

merged 46 commits into from
Aug 28, 2024

Conversation

yann300
Copy link
Contributor

@yann300 yann300 commented Jul 10, 2024

  • adds "Log in Github" to the File Explorer.
  • the github user is synced through an app state
  • tests are added to connect through the FE and disconnect

Copy link

netlify bot commented Jul 10, 2024

Deploy Preview for remixproject ready!

Name Link
🔨 Latest commit 2e79e85
🔍 Latest deploy log https://app.netlify.com/sites/remixproject/deploys/66cf05e8b5e4c5000862dc71
😎 Deploy Preview https://deploy-preview-4979--remixproject.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@yann300 yann300 added the ready-to-review PR ready to review label Jul 10, 2024
@LianaHus
Copy link
Collaborator

I would suggest redesigning a bit:
Screenshot from 2024-07-11 10-38-48
Screenshot from 2024-07-11 10-38-25

@LianaHus
Copy link
Collaborator

LianaHus commented Jul 11, 2024

  • IMO "Log in to GitHub" is enough we do not need "Click to" in the tooltip
  • I would suggest to use "Sign in" as a label and in tooltip instead of "Log in"

Screenshot from 2024-07-11 10-43-18

@@ -52,6 +53,7 @@ export const GitHubCredentials = () => {
setGithubToken(credentials.token || '')
setGithubUsername(credentials.username || '')
setGithubEmail(credentials.email || '')
props.plugin.emit('loggedInGithubChanged')
Copy link
Collaborator

Choose a reason for hiding this comment

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

no passing around plugin in props please.

Copy link
Collaborator

Choose a reason for hiding this comment

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

use plugin actions context

@yann300 yann300 force-pushed the github_fe branch 4 times, most recently from af8c99a to b2a850e Compare August 22, 2024 09:57
@bunsenstraat bunsenstraat marked this pull request as ready for review August 24, 2024 18:14
@bunsenstraat bunsenstraat removed their request for review August 24, 2024 18:15
@bunsenstraat bunsenstraat enabled auto-merge August 27, 2024 06:54
Copy link
Collaborator

@Aniket-Engg Aniket-Engg left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-08-27 at 7 07 38 PM
  1. It should not say Login in with
Screenshot 2024-08-27 at 7 12 40 PM
  1. It looks like manually providing Github credentials never works and show error dialog in each case.
Screenshot 2024-08-27 at 7 12 56 PM
  1. It is better to tell what permissions are expected with token. I couldn't make this work.
Screenshot 2024-08-27 at 7 14 59 PM
  1. For logged in user, labels and details are not properly aligned, even if there are a great space available
Screenshot 2024-08-27 at 7 21 29 PM
  1. Labels are inconsistent saying Git for username and email but GitHub for token

@bunsenstraat
Copy link
Collaborator

bunsenstraat commented Aug 27, 2024

@Aniket-Engg @yann300 please chip in on the topic

Some answers and explanations:

  1. It should not say Login in with

what should it say? Sign in is better?

  1. It looks like manually providing Github credentials never works and show error dialog in each case.

That is not true in my tests and is also included in e2e, it works when you provide a valid token with the correct permissions. The point is this a lot of instructions to give how to create a valid token. So where do we put it? in a popup? or a panel? where can we explain this?
I would rather dismiss the whole manual procedure all together and only allow login via github... but that would be too radical probably. 👯

  1. It is better to tell what permissions are expected with token. I couldn't make this work.

Yes it's technically not available. Github only supplies the scopes of permissions of a token in the API when you log in with github. Not when you provide tokens you create yourself. Nothing I can do about it. So we can only check the permissions when you login in, not when it's manual. I wish it was different.

  1. For logged in user, labels and details are not properly aligned, even if there are a great space available

It looks the same as in any other part of remix UI with input fields. To my eyes 👁️

  1. Labels are inconsistent saying Git for username and email but GitHub for token

Yes that is on purpose. the username/email is a git setting, the token is only connected to github. the username/email is something you always need to do commits, and is git only. you can commit and do stuff if you have that filled in. the token is optional.

@Aniket-Engg
Copy link
Collaborator

what should it say? Sign in is better?

Login in with GitHub can be updated to just Login with GitHub

@bunsenstraat bunsenstraat merged commit 74057bd into master Aug 28, 2024
32 checks passed
@bunsenstraat bunsenstraat deleted the github_fe branch August 28, 2024 11:38
@STetsing
Copy link
Collaborator

Would be nice to

  • click on 'log in` and land to login setting
  • access current setting when in explorer by clicking on the logged in profile

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

Successfully merging this pull request may close these issues.

5 participants