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.

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

VC Edit Profile #6273

Merged
merged 9 commits into from
May 30, 2024
Merged

VC Edit Profile #6273

merged 9 commits into from
May 30, 2024

Conversation

bobbykolev
Copy link
Contributor

Copy link
Contributor

@ccanos ccanos left a comment

Choose a reason for hiding this comment

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

just a few style comments, but logic wise looks good to me

@@ -13,11 +17,31 @@ export const VCSettingsPage = () => {
},
});

const [updateContributorMutation] = useUpdateVirtualContributorMutation();

const handleUpdate = async virtualContributor => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just call the function here, there's no need to await if you're not doing anything after

if (loading) return <Loading text={'Loading Virtual Contributor Settings ...'} />;

// TODO: StorageProvider for the VC
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we just use the platform StorageProvider for entities that we are creating (which is definitely wrong, but it is a solution if you need it now),
or hide the ReferencesSegment in the Create form and show it in the edit form. Which is also bad but I think it's better better.

};

const validationSchema = yup.object().shape({
name: nameSegmentSchema.fields?.name || yup.string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

probably copied from somewhere else but please use ?? for nullish coalescing


const HostFields = () => (
<>
<FormikInputField name={'host'} title={'Host'} required readOnly disabled />
Copy link
Contributor

Choose a reason for hiding this comment

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

please use double quotes here instead of {'host'} ...
All if this is temporary, right? can you add a comment?


const [handleSubmit, loading] = useLoadingState(async (values: VirtualContributorFromProps) => {
const { tagsets, description, tagline, name, ...otherData } = values;
const updatedTagsets = getUpdatedTagsets(tagsets || []);
Copy link
Contributor

Choose a reason for hiding this comment

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

|| => ?? same here

description,
tagline,
tagsets: updatedTagsets.map(r => ({
ID: r.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this id: undefined required? you're creating a new object...
also I would call the variable tagset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm displaying the tagsets, there's no default tagset. I could use some help on switching from default keywords and capabilities to just tags.

const result: UpdateTagset[] = [];
updatedTagsets.forEach(updatedTagset => {
const originalTagset = tagsets?.find(value => value.name === updatedTagset.name);
if (originalTagset) result.push({ ...originalTagset, tags: updatedTagset.tags });
Copy link
Contributor

Choose a reason for hiding this comment

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

please use curly brackets here

@bobbykolev bobbykolev requested a review from ccanos May 30, 2024 14:59
@bobbykolev bobbykolev merged commit c709f15 into client-5862 May 30, 2024
2 checks passed
@valentinyanakiev valentinyanakiev deleted the client-5911-edit-vc branch June 6, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants