🪟🔧 Remove top level state for unfinished flows in connector form #20135
Conversation
…or-form-loading-state
flash1293
added
team/extensibility
area/frontend
…or-form-name-override
…connector-form-unifinished-flows
….module.scss Co-authored-by: Tim Roes <tim@airbyte.io>
…or-form-loading-state
…irbytehq/airbyte into flash1293/connector-form-loading-state
…connector-form-name-override
…connector-form-unifinished-flows
…or-form-loading-state
…connector-form-name-override
…connector-form-unifinished-flows
…or-form-loading-state
…connector-form-name-override
…connector-form-unifinished-flows
…-form-loading-state
…connector-form-name-override
…connector-form-unifinished-flows
…-form-name-override
…connector-form-unifinished-flows
…-form-unifinished-flows
| value={field.value ?? ""} | ||
| type="password" | ||
| disabled={disabled} | ||
| error={error} |
There was a problem hiding this comment.
Something I noticed while testing: the secret input border is not set to red when it has an error
I think this is because the error prop is not being passed to this component from Control.tsx
There was a problem hiding this comment.
Good catch, seems like it's like that on master too. Should be fixed:
...e-webapp/src/views/Connector/ConnectorForm/components/Property/SecretConfirmationControl.tsx
Outdated
Show resolved
Hide resolved
| useEffect(() => { | ||
| if (!dirty && !touched && previousValue) { | ||
| setPreviousValue(undefined); | ||
| } | ||
| }, [dirty, helpers, previousValue, touched]); |
There was a problem hiding this comment.
Listen to the global formik state - if the dirty flag flips to false but there is a "previous value", reset that as well
Just to confirm my understanding: this code is basically ensuring that if the formik form is reset back to the initial state, then the previousValue state will be cleared out? An in practice, this would happen if a user clicks the Cancel button at the bottom of the ConnectorForm
There was a problem hiding this comment.
Exactly, if the form-level "Cancel" button is hit, all open secret controls are "closed" as well.
...e-webapp/src/views/Connector/ConnectorForm/components/Property/SecretConfirmationControl.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const component = | ||
| multiline && (isEditInProgress || !showButtons) ? ( | ||
| <SecretTextArea |
There was a problem hiding this comment.
I noticed a couple small styling differences with the text area component which I put up a small PR here to fix: #20397
|
Thanks for the review, @lmossman I addressed your comments and also fixed a bit the styling by adding a gap and top-aligning the cancel/done buttons instead of centering them (which looked really weird for textareas): |
Part of #14250
Based on #20132 (wait until merged)
What
This PR removes the top level state for "unfinished flows" (the confirm control to "open" a secret field and submit it) and pulls it into local component state.
This is one step towards removing the ui-widget state completely from the connector form to make it easier to reason about state handling as well as integrating it in other places.
How
SecretConfirmationControlas it's not used anywhere else) to avoid the local state leaking out of that componentfalsebut there is a "previous value", reset that as well🚨 User Impact 🚨
There is one small detail which changed based on this - the "Save changes and test" button would be disabled as long as the "unfinished flow" still exists. As there is no global state anymore to inform the button a secret is in the process of being edited, it will allow to "retest" even if the secret editing is not "finished" yet.
I don't see a UX problem with this change (especially as it's only affecting editing an existing connector but not creating a new one), but if there is a strong reason it can't work this way we can introduce a boolean top level state to pass this information around.