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

Upload an existing yaml file #324

Merged
merged 20 commits into from
Dec 3, 2024

Conversation

aevo98765
Copy link
Member

This feature was requested to allow users that had downloaded/curated partial yaml files to be able to upload their yaml and continue on using the UI.

@mingxzhao
Copy link
Member

Awesome thanks! Taking a look.

@aevo98765 aevo98765 force-pushed the issue/#241/upload-yaml-file branch from 51991ec to be6af92 Compare November 12, 2024 10:29
@aevo98765
Copy link
Member Author

Rebased with updated next.js (v15)

@aevo98765
Copy link
Member Author

Hey @nerdalert, @mingxzhao, @vishnoianil, rebase has been done and this one is ready for review when you guys get chance.

Copy link
Member

@vishnoianil vishnoianil left a comment

Choose a reason for hiding this comment

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

Overall PR looks good. Testing locally for both skill and knowledge and it works!!. Added minor comment and one suggestion on the UX.

src/components/Contribute/Knowledge/index.tsx Outdated Show resolved Hide resolved
/>
}
>
<FormGroup fieldId="text-file-with-restrictions-example">
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if this can be a simple button that says "Upload Existing Skill" that takes yaml/yml file as an input? Current form-group gives an impression that it's the part of the existing form.

@vishnoianil
Copy link
Member

@Misjohns This PR adds a feature where user can upload an existing skill/knowledge yaml file (in case they save the yaml to resume the work later). Current PR adds it as a form group (see below snapshot).

Screenshot 2024-11-15 at 12 27 15 AM

I am wondering if we should use a button that says "Upload Existing Yaml" button. What would you recommend here?

@vishnoianil
Copy link
Member

@aevo98765 We upgraded the main branch to pattternfly6 now, so you might have to rebase your branch over main, and give it a test run.

@Misjohns
Copy link
Collaborator

Misjohns commented Nov 15, 2024

So the proposal would be to add this to the contribution form? The user would still need to fill out the other fields in the form to submit? @aevo98765 @vishnoianil

@Misjohns
Copy link
Collaborator

I recommend placing this feature at the very top of the page. I have included my recommendations below. Once we settle on a direction I'll get someone from content to finesse the copy.

image

@vishnoianil
Copy link
Member

So the proposal would be to add this to the contribution form? The user would still need to fill out the other fields in the form to submit? @aevo98765 @vishnoianil

Yes, yaml will fill all the fields for which data is available in yaml, rest all field will be filled by the contributor (such as email/name, brief description etc).

@vishnoianil
Copy link
Member

I recommend placing this feature at the very top of the page. I have included my recommendations below. Once we settle on a direction I'll get someone from content to finesse the copy.

image

I am more inclined toward second option, because it's more visible, compared to link in first option.

Copy link
Member

@mingxzhao mingxzhao left a comment

Choose a reason for hiding this comment

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

LGTM, agreed to second option, looks much cleaner and clearer

@Misjohns
Copy link
Collaborator

One minor tweak to Option 2. We should make the YAML button a secondary button instead of a primary. The primary button is at the bottom of the form to submit.
image

…g a function to populate the knowledge form fields

Signed-off-by: Ash Evans <[email protected]>
@aevo98765 aevo98765 force-pushed the issue/#241/upload-yaml-file branch from b88d2ae to 97e5b8d Compare November 27, 2024 21:55
@aevo98765
Copy link
Member Author

Screenshot 2024-11-28 at 14 28 47
Screenshot 2024-11-28 at 14 28 58

@aevo98765
Copy link
Member Author

Hey guys, sorry this has taken a while. Please see the screenshots above of the new components. This is functioning and users can now upload complete or partial yaml files for both skills and knowledge. However, the error handling needs to be implemented. I don't have the bandwidth to do this in the next week or so. Therefore I have created a new issue and sel-assigned to sort this out.

#382

@aevo98765
Copy link
Member Author

@vishnoianil g2g on this one?

Copy link
Member

@vishnoianil vishnoianil left a comment

Choose a reason for hiding this comment

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

/LGTM

Awesome work as usual @aevo98765

@vishnoianil vishnoianil merged commit 0c188d4 into instructlab:main Dec 3, 2024
6 checks passed
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.

4 participants