-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: created basic section and their forms #355
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request implements a new feature for creating basic sections and their forms in the ecc-client-elixir-ro-crate package. The changes introduce a new LitElement-based component called ECCCLientRoCrateAbout, which creates a tabbed interface for displaying and editing information about RO-Crate metadata. The implementation uses the @elixir-cloud/design library for form components and defines several form fields for different sections of the RO-Crate metadata. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sivangbagri - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider improving naming consistency. The main class name
ECCCLientRoCrateAbout
should likely beECCClientRoCrateAbout
, and there's a mix of snake_case and camelCase in the code that could be standardized. - The field definitions are currently hardcoded. Consider loading these from a configuration file or API for better flexibility and maintainability.
- There are several TODO comments in the code. It's generally better to resolve these before submitting a pull request, or at least provide more context about why they're left as TODOs.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
import "@elixir-cloud/design/dist/components/form/index.js"; | ||
import { Field } from "@elixir-cloud/design/dist/components/form/form"; | ||
|
||
export default class ECCCLientRoCrateAbout extends LitElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (typo): Fix typo in class name
The class name 'ECCCLientRoCrateAbout' contains a typo. It should be 'ECCClientRoCrateAbout' for consistency and clarity.
export default class ECCClientRoCrateAbout extends LitElement {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also true!
fieldOptions: { | ||
required: true, | ||
default: "Dataset", | ||
tooltip: "Your house number", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Update or remove irrelevant tooltip
The tooltip for the 'Type' field mentions 'Your house number', which seems unrelated. Consider updating this to a relevant description or removing it if not needed.
tooltip: "Your house number", | |
tooltip: "The type of dataset being described", |
}, | ||
}, | ||
{ | ||
key: "pulisher", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (typo): Correct typo in 'publisher' key
There's a typo in the 'publisher' key. It's currently spelled as 'pulisher', which could cause issues when accessing or setting this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect Spelling!
@Salihu @anuragxxd Kindly review. Ignore naming issues , I will fix it |
hi @sivangbagri, I'm having a bit of trouble reviewing your code, can you please create an issue that goes into detail about the goals of this project and how you are going about achieving said goals. Also include important links like the schema for the version of ro-crate you're working with, and links to any referenced projects or profiles. If there is one already please link it in the PR description. |
@SalihuDickson Hii , check this out : #356 . If anything else is needed please let me know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small changes otherwise lgtm
}, | ||
}, | ||
{ | ||
key: "pulisher", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect Spelling!
import "@elixir-cloud/design/dist/components/form/index.js"; | ||
import { Field } from "@elixir-cloud/design/dist/components/form/form"; | ||
|
||
export default class ECCCLientRoCrateAbout extends LitElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also true!
@anuragxxd new commit has required changes along with improved form structure of about and hasPart sections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few things:
- The Field is
@type
, notType
- The
@id
field should be required - What is the "about dataset" section for? Are the fields above that not for the root data (the dataset) entity.
Some discrepancies between from the latest official Ro-Crate sepcification:
- The
@type
field is not needed as both the metadata and the dataset entities have static types, you only need the@type
field when you are creating a new entity. - Name, description and date published should be required, there is also a required license entity that you need to add fields for.
There are also a bunch more fields that need to be added for the dataset entity, but we can first get the required fields set up and incrementally reach achieve full support in subsequent PRs. Please create an issue stating that we need to add full support for all accepted fields in the dataset entity, we can then begin to list the fields, their categories and associated entities as well as implementation details there.
Great work so far @sivangbagri, please take a look at the specification for the latest version and make necessary adjustments. And if you have any questions please feel free to reach out.
The fields you've added for the Author, Publisher and Funder fields won't really work, based on the minimum requirements for the Ro-Crates. The author object is would be an instance of a people entitiy. perhaps the "Related People, Orgs & Works" page can contain all contextual entities, but we would need a better design to make that clear, as the current structure implies several disjointed entities. This is a lot at this point, my suggestion would be that you convert this to a draft and create a new PR solely adding fields for the metadata entity, another for the dataset entity and then we can address these other objects and entities in subsequent PRs. |
Co-authored-by: salihuDickson <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]> Co-authored-by: salihuDickson <[email protected]>
…oud-aai#342) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Pratiksha Sankhe <[email protected]>
…ud-components into ro-crate" This reverts commit 5c56b96, reversing changes made to a6e5aff.
This reverts commit a6e5aff.
This reverts commit c88869a.
ac3d608
to
d39e6f1
Compare
I was in the process of trying to revert the ro-crate changes and merge the build fixes from earlier but i just realized those have already been implemented on main. I think you can go ahead and close this PR. |
Description
Fixes #(issue)
Checklist
Comments
Summary by Sourcery
Add a new 'ECCCLientRoCrateAbout' component with tabbed forms for managing metadata sections, and provide demo HTML files for component demonstration.
New Features:
Enhancements:
Documentation: