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

Add logout menu option #27

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Add logout menu option #27

merged 1 commit into from
Jun 15, 2023

Conversation

Daeraxa
Copy link
Member

@Daeraxa Daeraxa commented Apr 25, 2023

Adds a logout menu item to Packages > GitHub.
Stems from a conversation on Discord where three of us couldn't work out how to log out once the token was entered other than manually deleting it from the keyring. This isn't a perfect solution (we could probably add a button to the main panel or something) but at least it isn't hidden purely behind a command and can be found by looking where people might reasonably look for such an option.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

While I realize the tests here are all failing, I really don't think that has anything to do with this PR.

Adding it here is an important quality of life improvement.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested and working.

Test notes:

When I'm logged in to the GitHub package and I use this menu item, (or the command from the command palette, effect is the same), I am successfully logged out of the GitHub package.

Add a warning before actually logging out?:

There's no warning, I think there should be, but I don't see a need to block this PR on that. Adding a warning is way harder, and the command already existed before, this is just making a more GUI point-and-click friendly way to log out. But making a potential pitfall easier to access does make me hope that some day we will have a warning dialog before actually logging out.

EDIT to say more clearly: The pitfall is forgetting and potentially having to re-roll your token, not just the minor inconvenience of being logged out. Logging out deletes the token off the user's system. At least it did for me on macOS. So folks who didn't actually remember or know how to re-add a token may be locked out until they can understand how to get their token added back to Pulsar's GitHub integration again. Hence why it would be worth a warning first, IMO.

Screenshots

Screenshots of the menu item as it appears on macOS Catalina (10.15):

Screenshot before (click to expand):

Before, Pulsar's menus, with "Packages > GitHub" highlighted. There is no "Log Out" item in the menus.

Screenshot after (click to expand):

After, Pulsar's menus, with "Packages > GitHub > Log Out" highlighted.

CI?

CI at this repo has been broken since we forked, as far as I can tell/remember. This PR is working fine in the very small area it touches, so I am confident this isn't gonna make or break our CI situation / shouldn't be relevant to the CI status of this repo IMO.

@DeeDeeG
Copy link
Member

DeeDeeG commented Jun 15, 2023

Two approvals, I am merging.

I think I can do the release tagging song and dance before we do a (planned anyway) Pulsar release later today.

BTW: Instructions for how I've been tagging the releases has been in the commit messages for commits titled "lib: Pre-transpile the lib/ JS files", which you can find in the commit history under the "pretranspiled" tags in this repo.

Such as (see instructions here): https://github.com/pulsar-edit/github/releases/tag/v0.36.14-pretranspiled

@DeeDeeG DeeDeeG merged commit 56e1031 into pulsar-edit:master Jun 15, 2023
0 of 5 checks passed
@Daeraxa
Copy link
Member Author

Daeraxa commented Jun 15, 2023

I agree about the logout popup but as you say that is a far larger thing. It also depends on what we want to do with this package in general as there were/are discussions about moving to a more "generic" forge package etc. so it may well need to deal with logging out different services.

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.

None yet

3 participants