-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🪟🎉 Connector form: Use proper validation in array section #20725
Conversation
…nector-form-types
…nector-form-types
…or-form-loading-state
…or-form-name-override
…connector-form-unifinished-flows
…ector-form-remove-ui-widget-state
….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
…-form-remove-ui-widget-state
…lash1293/array-validation
This PR title probably shouldn't start with |
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.
A couple small comments but neither blocking, LGTM. Tested locally and seems to work as expected
onStartEdit={setEditIndex} | ||
onStartEdit={(n) => { | ||
setEditIndex(n); | ||
setItemInEdit(items[n]); |
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.
Do we not need to create a copy of this?
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.
No, formik takes care of not mutating any objects in the form values, it's always shallow-copying.
const [editIndex, setEditIndex] = useState<number>(); | ||
const [field, , fieldHelper] = useField<Array<Record<string, string>>>(path); | ||
const [editIndex, setEditIndex] = useState<number | undefined>(); | ||
const [itemInEdit, setItemInEdit] = useState<Record<string, string> | undefined>(); |
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.
Initially I thought itemInEdit
contained the new value of the item, and it took me a bit to understand that this is the old value of the item from before it was edited.
Maybe a name like uneditedItem
, oldItem
, prevItem
would make this a bit more clear?
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 point, changed
I did, but I typed connector builder too many times in the last two weeks. |
…lash1293/array-validation
…lash1293/array-validation
What
Fixes #20234
How
Instead of holding the state of the modal form in a separate place, edit in in-place so formik validation just works.
In order to keep the form behind the UI stable and to be able to roll back on the user hitting the Cancel button, the ArraySection pulls the item in edit into a local state and renders the list of items based on that.
On submitting the modal form, that local state is thrown away.
Based on top of the refactoring PR because I was running into weird issues with the unit tests which do not occur after the refactoring.