Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(557) - Enforce required name and type in beans forms #832

Merged
merged 1 commit into from
Feb 19, 2024
Merged

fix(557) - Enforce required name and type in beans forms #832

merged 1 commit into from
Feb 19, 2024

Conversation

tplevko
Copy link
Contributor

@tplevko tplevko commented Feb 13, 2024 •
edited
Loading

fixes #557

vokoscreenNG-2024-02-13_12-36-35.mp4

@tplevko tplevko force-pushed the issue_557 branch 4 times, most recently from 96f3501 to f5e9297 Compare February 13, 2024 14:46
@tplevko tplevko marked this pull request as draft February 13, 2024 16:11
@tplevko tplevko force-pushed the issue_557 branch 2 times, most recently from 53204d8 to 2ef7776 Compare February 15, 2024 14:59
@tplevko tplevko marked this pull request as ready for review February 16, 2024 06:40
@lhein lhein requested a review from lordrip February 16, 2024 06:51
lordrip
lordrip approved these changes Feb 16, 2024
View reviewed changes
Copy link
Member

@lordrip lordrip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job @tplevko , we just need to remove the submitRef references and we're good to go

packages/ui/src/components/Form/CustomAutoForm.tsx Outdated
@@ -12,6 +12,8 @@ interface CustomAutoFormProps extends IDataTestID {
omitFields?: string[];
onChangeModel?: (model: unknown) => void;
onChange?: (path: string, value: unknown) => void;
handleConfirm?: () => void;
submitRef?: React.RefObject<HTMLButtonElement>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The submitRef property is not used in this component 😃

Suggested change
submitRef?: React.RefObject<HTMLButtonElement>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catching that - a residue from my previous attempt :D

packages/ui/src/components/Form/bean/NewBeanModal.tsx Outdated
props.onCreateBean(beanModel as RegistryBeanDefinition);
const handleConfirm = useCallback(async () => {
// validation updates the bean model, so we need to clone it to avoid creating the bean with default values
const beanModelTmp = { ...beanModel };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, we're just copying the outer object, if beanModel has nested properties, those would still have the new value, so should use either cloneDeep from lodash or structuredClone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think beansModel have nested properties, like constructors or properties
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, sounds like this could cause issues - updated

packages/ui/src/components/Form/bean/NewBeanModal.tsx Outdated
// validation updates the bean model, so we need to clone it to avoid creating the bean with default values
const beanModelTmp = { ...beanModel };
const valid = await submitRef.current?.form.validate();
if (valid === undefined || valid === null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[pro-tip]: We have the isDefined util to validate these two options at once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will try to remember this utility, looks better now.

packages/ui/src/components/MetadataEditor/MetadataEditor.tsx Outdated
@@ -15,9 +15,11 @@ interface MetadataEditorProps {
metadata: any;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
onChangeModel: (model: any) => void;
handleConfirm?: () => void;
submitRef?: React.RefObject<HTMLButtonElement>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this one

Suggested change
submitRef?: React.RefObject<HTMLButtonElement>;
submitRef?: React.RefObject<HTMLButtonElement>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed, thanks!

packages/ui/src/components/MetadataEditor/MetadataEditor.tsx Outdated
@@ -121,6 +128,8 @@ export const MetadataEditor: FunctionComponent<PropsWithChildren<MetadataEditorP
disabled={isFormDisabled()}
sortFields={true}
ref={fieldsRefs}
handleConfirm={props.handleConfirm}
submitRef={props.submitRef}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And same here 😃

Suggested change
submitRef={props.submitRef}

packages/ui/src/components/MetadataEditor/MetadataEditor.tsx Outdated
@@ -135,8 +144,10 @@ export const MetadataEditor: FunctionComponent<PropsWithChildren<MetadataEditorP
disabled={isFormDisabled()}
sortFields={true}
ref={fieldsRefs}
handleConfirm={props.handleConfirm}
submitRef={props.submitRef}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here

Suggested change
submitRef={props.submitRef}

@lhein lhein merged commit 90205ee into KaotoIO:main Feb 19, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igarashitm igarashitm igarashitm left review comments

@lordrip lordrip lordrip approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enforce required name and type in beans forms
4 participants
@tplevko @igarashitm @lordrip @lhein