feat: RO-Crate Contextual entity #373
Conversation
Reviewer's Guide by SourceryThis PR implements contextual entity support (Person and Organization) in the RO-Crate client by introducing a new tabbed form component. The implementation uses Lit Element and includes three main sections: About, Related People/Organizations, and Structure. The form dynamically updates based on user selections and handles different types of licenses and entity relationships. 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
+ arrayOptions: object
+ groupOptions: object
+ children: Field[]
}
ECCClientRoCrateAbout --> Field
note for ECCClientRoCrateAbout "This class represents a LitElement component with tabbed forms for contextual entities."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![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:
- There appears to be a bug where organization types default to 'Person' instead of 'Organization' in the publisher and funder sections
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue 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.
| const licenceFieldIndex = this.AboutFields.findIndex( | ||
| (field) => field.key === "licence" | ||
| ); | ||
| const licenceChildren = this.AboutFields[licenceFieldIndex].children ?? []; |
There was a problem hiding this comment.
suggestion (performance): Consider combining the multiple .filter() operations into a single pass for better performance and readability
Instead of chaining multiple .filter() calls, you could combine the conditions using logical operators in a single filter operation. This would improve both performance and code maintainability.
| label: "@type", | ||
| type: "text", | ||
| fieldOptions: { | ||
| default: "Person", |
| private _switchTab(index: number): void { | ||
| this.activeTab = index; | ||
| } | ||
| private _handleDataset(e: CustomEvent): void { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring license field handling to use a configuration map pattern
The license field handling in _handleDataset can be simplified by using a configuration map approach. This reduces nesting and makes the code more maintainable:
private readonly LICENSE_CONFIGS = {
URL: {
removeKeys: ['@id', '@type', 'name', 'desc'],
addFields: [{
key: 'url',
label: 'URL',
type: 'url',
fieldOptions: { required: true }
}]
},
CreativeWork: {
removeKeys: ['url'],
addFields: [{
key: '@id',
label: '@id',
type: 'text',
fieldOptions: {
required: true,
tooltip: 'Persistent, managed unique ID in URL format'
}
},
// ... other CreativeWork fields
]
}
};
private _handleDataset(e: CustomEvent): void {
if (e.detail.key !== 'licence') return;
const config = this.LICENSE_CONFIGS[e.detail.value];
if (!config) return;
const licenceFieldIndex = this.AboutFields.findIndex(field => field.key === 'licence');
if (licenceFieldIndex === -1) return;
const licenceChildren = this.AboutFields[licenceFieldIndex].children ?? [];
const updatedChildren = licenceChildren
.filter(child => !config.removeKeys.includes(child.key))
.concat(config.addFields);
this.AboutFields = [
...this.AboutFields.slice(0, licenceFieldIndex),
{ ...this.AboutFields[licenceFieldIndex], children: updatedChildren },
...this.AboutFields.slice(licenceFieldIndex + 1)
];
}This approach:
- Moves field configurations to a declarative map
- Eliminates nested conditionals
- Reduces repeated array operations
- Makes adding new license types easier
anuragxxd
force-pushed
the
main
branch
3 times, most recently
from
1845262 to
1395942
Compare
| }, | ||
| children: [ | ||
| { | ||
| key: "person", |
There was a problem hiding this comment.
according to the ro-crate specification Author can either be a person or organization. So you should not label this as person. I think it would be better to use the key 'author-entities' and the label 'Author Entities'.
Also please the keys for the other fields in this array accordingly.
| }, | ||
| }, | ||
| { | ||
| key: "personType", |
There was a problem hiding this comment.
the type of type allows for either organization or person, you should change this to a select field with only those 2 options.
| }, | ||
| children: [ | ||
| { | ||
| key: "org", |
There was a problem hiding this comment.
The same issue I addressed above applies here as well. The publisher entity allows for organization or person. Please change the keys and labels accordingly. And change the type field to a select field that allows for either 'organization' or 'person' to be selected.
| }, | ||
| children: [ | ||
| { | ||
| key: "org", |
There was a problem hiding this comment.
The same issue I stated above applies here, please update the entity to reflect the specification. It is pretty much the same situation as the author and publisher entitties.
|
@sivangbagri, could you please modularize your code a little, one suggestion would be to move the fields objects into a Also you can go ahead an resolve conflicts with the main branch |
Description
This PR adds contextual entity (Person, Org) support to ro-crate.
Continuing #370
Checklist
Comments
Summary by Sourcery
New Features: