Ro-Crate Dataset entity #364
Conversation
Co-authored-by: salihuDickson <[email protected]>
Signed-off-by: Shivang Bagri <[email protected]>
Signed-off-by: Salihu <[email protected]>
…onfig.js Signed-off-by: Salihu <[email protected]>
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request implements a new component for handling RO-Crate Dataset entities in the ELIXIR Cloud & Compute project. The changes include adding required fields for the root dataset entity, implementing a form for dataset metadata, and enhancing the existing form component with new event handling capabilities. Sequence diagram for form event handlingsequenceDiagram
actor User
participant ECCUtilsDesignForm
participant ECCClientRoCrateAbout
User ->> ECCUtilsDesignForm: Interact with form
ECCUtilsDesignForm -->> ECCClientRoCrateAbout: Dispatch "ecc-utils-change" event
ECCClientRoCrateAbout ->> ECCClientRoCrateAbout: _handleDataset(e)
ECCClientRoCrateAbout -->> ECCClientRoCrateAbout: Update AboutFields based on event detail
ECCClientRoCrateAbout -->> ECCUtilsDesignForm: Update form display
Class diagram for ECCClientRoCrateAbout componentclassDiagram
class ECCClientRoCrateAbout {
- activeTab: number
- AboutFields: Field[]
+ render() void
+ _switchTab(index: number) void
+ _handleDataset(e: CustomEvent) void
}
class Field {
+ key: string
+ label: string
+ type: string
+ fieldOptions: object
+ children: Field[]
}
ECCClientRoCrateAbout --> Field
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @sivangbagri - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider refactoring the ECCClientRoCrateAbout class in the future to break it down into smaller, more manageable components. This would improve maintainability without affecting current functionality.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -129,6 +129,16 @@ export default class EccUtilsDesignForm extends LitElement { | |||
| @sl-change=${(e: Event) => { | |||
| _.set(this.form, path, (e.target as HTMLInputElement).checked); | |||
| this.requestUpdate(); | |||
| this.dispatchEvent( | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the repeated event dispatch code into a separate function.
The repeated event dispatch code across different form elements introduces unnecessary complexity. To simplify the code and improve maintainability, extract this common behavior into a separate function. Here's an example of how you can refactor this:
// Add this utility function
private dispatchChangeEvent(key: string, value: any) {
this.dispatchEvent(
new CustomEvent("ecc-utils-change", {
detail: { key, value },
bubbles: true,
composed: true,
})
);
}
// Then, replace the repeated code in each event handler with a call to this function
// For example, in the switch element:
@sl-change=${(e: Event) => {
const checked = (e.target as HTMLInputElement).checked;
_.set(this.form, path, checked);
this.requestUpdate();
this.dispatchChangeEvent(field.key, checked);
}}
// Similarly for other elements, e.g., file input:
@change=${async (e: Event) => {
const { files } = e.target as HTMLInputElement;
_.set(this.form, path, files);
this.requestUpdate();
this.dispatchChangeEvent(field.key, files);
}}This refactoring reduces code duplication, improves maintainability, and makes it easier to modify the event dispatch behavior in the future if needed. It also retains all the new functionality while simplifying the code structure.
| private _switchTab(index: number): void { | ||
| this.activeTab = index; | ||
| } | ||
| private _handleDataset(e: CustomEvent): void { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the _handleDataset method and moving field definitions to a separate file.
The _handleDataset method can be simplified to reduce complexity and improve maintainability. Here's a suggested refactoring:
- Separate the field update logic:
private updateLicenceFields(licenceType: string): Field[] {
const baseFields = [
{ key: "licence", label: "Licence Type", type: "select", fieldOptions: { required: true },
selectOptions: [{ label: "URL", value: "URL" }, { label: "CreativeWork", value: "CreativeWork" }]
}
];
const urlFields = [
{ key: "url", label: "URL", type: "url", fieldOptions: { required: true } }
];
const creativeWorkFields = [
{ key: "@id", label: "@id", type: "text", fieldOptions: { required: true, tooltip: "Persistent, managed unique ID in URL format" } },
{ key: "@type", label: "@type", type: "text", fieldOptions: { default: "CreativeWork", required: true, tooltip: "The type of the entity." } },
{ key: "name", label: "Name", type: "text", fieldOptions: { required: true, tooltip: "The name of the item." } },
{ key: "desc", label: "Description", type: "text", fieldOptions: { required: true, tooltip: "A description of the item." } }
];
return [...baseFields, ...(licenceType === "URL" ? urlFields : creativeWorkFields)];
}
private _handleDataset(e: CustomEvent): void {
if (e.detail.key !== "licence") return;
const licenceFieldIndex = this.AboutFields.findIndex(field => field.key === "licence");
if (licenceFieldIndex === -1) return;
const updatedLicenceField = {
...this.AboutFields[licenceFieldIndex],
children: this.updateLicenceFields(e.detail.value)
};
this.AboutFields = [
...this.AboutFields.slice(0, licenceFieldIndex),
updatedLicenceField,
...this.AboutFields.slice(licenceFieldIndex + 1)
];
}- Move field definitions to a separate file:
Create a new file fieldDefinitions.ts:
import { Field } from "@elixir-cloud/design/dist/components/form/form";
export const AboutFields: Field[] = [
// ... (move the AboutFields array here)
];
export const RelatedPeopleFields: Field[] = [
// ... (move the RelatedPeopleFields array here)
];
export const StructureFields: Field[] = [
// ... (move the StructureFields array here)
];Then import and use these definitions in your component:
import { AboutFields, RelatedPeopleFields, StructureFields } from './fieldDefinitions';
export default class ECCClientRoCrateAbout extends LitElement {
@state()
private AboutFields = AboutFields;
static RelatedPeopleFields = RelatedPeopleFields;
static StructureFields = StructureFields;
// ... rest of the class
}These changes will significantly reduce the complexity of the _handleDataset method, improve code organization, and make the component more maintainable and easier to extend in the future.
|
hi @sivangbagri great work so far, but it seems you're continuing from the #355 PR. I think we need to split up the PR, have one implementing the dataset entity, one for the metadata entity and then the necessary contextual and data entities respectively. That way we can make the PRs more specific and easier to review. You can go ahead and start with a new PR and just copy over the code implementing the dataset entity. I'll just cherry-pick the original PR to implement the fixes we made that were blockers for you. |
anuragxxd
force-pushed
the
main
branch
3 times, most recently
from
1845262
to
1395942
Compare
Description
Added required fields for root dataset entity.
Checklist
Summary by Sourcery
Add a new
ECCClientRoCrateAboutcomponent to manage dataset metadata with fields for ID, type, name, description, date published, and licence. Enhance theEccUtilsDesignFormcomponent to dispatch custom events on field changes. Introduce demo pages to showcase the new component.New Features:
ECCClientRoCrateAboutfor managing dataset metadata, including fields for ID, type, name, description, date published, and licence.ecc-client-elixir-ro-cratepackage to showcase theECCClientRoCrateAboutcomponent.Enhancements:
EccUtilsDesignFormcomponent to dispatch a custom eventecc-utils-changewhenever form fields are updated, allowing for better integration and event handling.