-
Notifications
You must be signed in to change notification settings - Fork 89
VSCODE-97: Connect with code lenses #154
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
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.
Nice!!! This will be some nice new functionality for playgrounds. Added comments on adding connections / vs reusing existing connections and a thought on possibly improving some of the messaging.
src/editors/playgroundController.ts
Outdated
|
||
return this._connectionController.parseNewConnectionAndConnect( | ||
selectedQuickPickItem.data.connectionModel | ||
); |
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.
How do you feel about this code living in the connectionController
vs playgroundController
?
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 don't have a strong opinion about it, so I don't mind moving it to the connection controller!
const message = `Currently connected to ${name}`; | ||
const message = activeConnection | ||
? `Currently connected to ${this._connectionController.getActiveConnectionName()}. Click here to change connection.` | ||
: 'Disconnected. Click here to add connection.'; |
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.
Maybe Disconnected. Click here to connect.
? I'm also thinking we could give the user some feedback while they're connecting. Maybe we can add a case for if the connection controller is currently connecting?
let message;
if (this._connectionController.isConnecting()) {
message = 'Connecting...'
} else if (this._connectionController.getActiveDataService()) {
message = `Currently connected to ${this._connectionController.getActiveConnectionName()}. Click here to change connection.`;
} else {
message = 'Disconnected. Click here to connect.';
}
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.
The current message is from design, but I like your option more! I will change it then and later show Claudia to double-check with her. Also, I like the idea of showing 'Connecting...' message, will add it as well 👍
src/editors/playgroundController.ts
Outdated
} | ||
|
||
return this._connectionController.parseNewConnectionAndConnect( | ||
selectedQuickPickItem.data.connectionModel |
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 this creates a new connection with each connection. I think when the user clicks on an existing connection we want to connect to that connection and not recreate a saved connection.
Maybe we can use connectWithConnectionId
on the connectionController
instead?
return this._connectionController.parseNewConnectionAndConnect(
selectedQuickPickItem.data.connectionId
);
and above we can change the data
property to connectionId
instead of connectionModel
.
@Anemy thanks for such good comments in the review! Pushed updates ✔️ |
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.
Good work - one small thing on the sorting then lgtm!
I think having the code in connectionController
might help us if we want to reuse it somewhere (maybe with the new connect screen?) nice move.
}; | ||
|
||
return codeLens; | ||
return [codeLens]; |
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.
cool! I didn't know we could skip the resolve code lens
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.
Me too! But I wanted to have all data preparation in provideCodeLenses for testing purposes and it worked! :)
src/connectionController.ts
Outdated
return -1; | ||
} | ||
if (collectionA.name > collectionB.name) { | ||
return 1; |
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 think this sort might be doing not quite what we expect:
Looks like we're comparing name
when the object builds the name into label
, I think if we use label
in the comparison it should be good.
- edit I'm also not sure if this comparison is case insensitive - maybe we want to use
localeCompare
:
(collectionA.label || '').localeCompare(collectionB.label || '')
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.
Interesting. I wanted it to be sorted the same way as in the sidebar. I assumed that it happens in databaseTreeItem.ts
in the getChildren
method that sorts by name. Is it some different list? Where we sort sidebar connections in 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.
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.
Np I will fix both places!
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.
Pushed updates, now connection quick pics and collections are case insensitive.
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.
🔥 🔥
Description
Use code lenses to connect to the new or saved connection:
Checklist
Motivation and Context
Types of changes