-
Notifications
You must be signed in to change notification settings - Fork 234
feat(connection-form): up to three buttons: save, connect, save & connect COMPASS-8360 #6341
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
} | ||
} | ||
onSaveAndConnectClicked={ | ||
disableEditingConnectedConnection |
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 changed all these to not include the button callback prop if it shouldn't show up. That makes the logic easier and less error prone because you don't have to check both the boolean AND the button to know which buttons to include inside the form and to know which one to call.
disableEditingConnectedConnection
is then only used when a button isn't directly involved like when showing the banner.
|
||
onCancel?: () => void; | ||
|
||
// If onSaveClicked is specified, a Save button will show up. It will be the |
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 comments are probably redundant now that the logic has been simplified and all buttons do what they say. But maybe they can guard against that regressing.
const onSubmitForm = useCallback( | ||
(intendedAction?: 'connect' | 'save-and-connect') => { | ||
const updatedConnectionOptions = cloneDeep(connectionOptions); | ||
// TODO: this method throws on malformed connection strings instead of |
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.
Again: This is an old comment.
showCSFLE: true, | ||
showProxySettings: true, | ||
saveAndConnectLabel: 'Connect', | ||
saveAndConnectLabel: 'Save & 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.
This is basically the magic that means this PR shouldn't break vscode. The callback was already onSaveAndConnectClicked and my understanding is that vscode already works this way where clicking the primary button in the edit form when editing a non-connected connection will just update everything and 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.
just fyi, VSCode provides it's own settings, it isn't using this hook. in case that's what you meant by this comment, not sure if I got it right. https://github.com/mongodb-js/vscode/blob/bfa57d4ce7b057fadb9fcec95327826f5d0dbb6b/src/views/webview-app/connection-form.tsx#L103
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.
That's what I meant - it uses the setting too. The setting is arguably redundant now, though: both apps set it to the same.
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.
One suggestion, otherwise looks good
packages/compass-sidebar/src/components/multiple-connections/sidebar.tsx
Outdated
Show resolved
Hide resolved
4e1fd28
to
c374ecf
Compare
This follows on from #6336
new connection
editing a disconnected connection
editing a connected connection
All the buttons do what they say.
The Save&Connect button's label already comes from a setting, so vscode should be unaffected.
Which buttons show up is now entirely determined by which of the three optional callbacks are specified. The onSubmit handler follows the same logic. Save & Connect is the default and Connect is there for the use case where people don't want their credentials to be saved with the connection.