-
Notifications
You must be signed in to change notification settings - Fork 234
feat(connection-form): allow editing connected connections, but only the name, colour and favorite checkbox COMPASS-8160 #6336
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
data-testid="save-button" | ||
variant={ButtonVariant.PrimaryOutline} | ||
variant={ | ||
onSaveAndConnect |
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.
Basically: If there is no save&connect button (the Connect button), make the Save button the "default" (ie. solid green) one. type for both are still implicitly "button", though. there is no default. Would be ambiguous anyway because we have a custom onSubmit handler that would have to be kept in sync.
); | ||
const onSubmitForm = useCallback(() => { | ||
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.
This is an old TODO comment. I just swapped two callbacks around.
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.
@paula-stacho Did we ever add a ticket for this TODO? Are we going to address it?
what happens when you click a submit button) you have to use submit() which only | ||
works with role="form", but eslint doesn't like that because role="form" | ||
should be redundant. | ||
https://stackoverflow.com/questions/59362804/pressing-enter-to-submit-form-in-react-testing-library-does-not-work |
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.
If there's a better way to simulate submitting a form by pressing enter rather than pressing a button so that the callback gets called in tests, please let me know. I added tests because pressing enter wasn't doing what pressing Save did so I modified the onSubmit callback, then realised we never actually seem to test this distinction.
<H3 className={headingWithHiddenButtonStyles}> | ||
{initialConnectionInfo.favorite?.name ?? 'New Connection'} | ||
{(initialConnectionInfo.favorite?.name ?? 'New Connection') || | ||
'Edit 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.
const sectionDarkModeStyles = css({ | ||
'--theme-background-color': palette.blue.dark3, | ||
'--theme-border-color': palette.blue.dark2, | ||
'--theme-background-color': palette.gray.dark3, |
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.
Note that according to the design, the blue help text boxes on the right are now grey.
darkMode?: boolean; | ||
initialConnectionInfo: ConnectionInfo; | ||
connectionErrorMessage?: string | null; | ||
disableEditingConnectedConnection?: boolean; |
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.
Optional new prop with undefined acting like false, so vscode should be unaffected.
> | ||
<div className={disabledConnectedConnectionContentStyles}> | ||
<div> | ||
While connected, you may only personalize your |
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 isn't really what "personalize" means, unless we consider connections to be people
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.
Fits the dictionary definition.
See figma design and slack thread.