-
Notifications
You must be signed in to change notification settings - Fork 303
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 account switcher component in the Accounts webview tab #6159
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.
Looks like a nice step forward. Some comments inline. TODOs may be appropriate at this point since this will be under heavy change for a while.
const tokenSource = message.value | ||
? 'paste' | ||
: await secretStorage.getTokenSource(serverEndpoint) | ||
const validationResult = await authProvider.validateAndStoreCredentials( |
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 know we used to be saving the endpoint here, I assume now we are talking to the network now too... Can we show a spinner and when the result comes back, flip the UX at that point?
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.
export const AccountSwitcher: React.FC<{ endpoints: string[] }> = ({ endpoints }) => { | ||
const [isOpen, setIsOpen] = useState(false) | ||
|
||
const onKeyDownInPopoverContent = useCallback<KeyboardEventHandler<HTMLDivElement>>( |
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.
Unless this is passed to some memoized thing, it is probably not worth memoizing... particularly since you're not memoizing onOpenChange
or the onclick
handler...
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.
Done.
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.
Looks good as a temporary solution until we built the unified design
Thanks @dominiccooney . |
nice typescript skills! |
9b4bc26
to
ecf0cc7
Compare
lgtm |
Changes
That PR:
Switch Account
button on the Accounts tab whenaccountSwitchingInWebview
capability is enabled in client.Test plan
Testing with jetbrains
main
:Fixes https://linear.app/sourcegraph/issue/QA-150
Manage Accounts
Testing with jetbrains migrated to webview account management:
Account switching:
Signing out:
Account adding:
Note: If wrong endpoint is provided web redirection will fail, but no error is displayed in the UI.
This definitely can be improved.
Account adding with token:
Invalid access token.