Skip to content

Conversation

@sivangbagri
Copy link
Contributor

@sivangbagri sivangbagri commented Oct 24, 2024

Description

Dedicated PR for RO-Crate Dataset entity

Checklist

  • My code follows the contributing guidelines of this project.
  • I am aware that all my commits will be squashed into a single commit, using the PR title as the commit message.
  • I have performed a self-review of my own code.
  • I have commented my code in hard-to-understand areas.
  • I have updated the user-facing documentation to describe any new or changed behavior.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have not reduced the existing code coverage.

Comments

Summary by Sourcery

Add a new RO-Crate Dataset entity component to the ecc-client-elixir-ro-crate package, featuring a tabbed interface for managing dataset metadata, related entities, and structure. Include demo HTML files to demonstrate component usage.

New Features:

  • Introduce a new RO-Crate Dataset entity component in the ecc-client-elixir-ro-crate package, allowing for detailed dataset metadata management.

Enhancements:

  • Implement a tabbed interface for managing dataset metadata, related people, organizations, and structure within the RO-Crate component.

Documentation:

  • Add demo HTML files to showcase the usage of the new RO-Crate Dataset entity component.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Oct 24, 2024

Reviewer's Guide by Sourcery

This PR implements a new RO-Crate Dataset entity component using LitElement. The component provides a tabbed interface for managing dataset metadata, including basic information, related entities, and structure. It features dynamic form handling with field validation and conditional rendering based on user input.

Class diagram for ECCClientRoCrateAbout component

classDiagram
    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 for managing RO-Crate Dataset metadata with a tabbed interface."
Loading

File-Level Changes

Change Details Files
Implementation of the main Dataset entity component with tabbed interface
  • Created a LitElement-based component with three tabs: About, Related People, and Structure
  • Implemented tab switching functionality
  • Added state management for active tab tracking
packages/ecc-client-elixir-ro-crate/src/components/about/about.ts
Form field definitions and dynamic form handling
  • Defined required dataset fields including @id, @type, name, description, and datePublished
  • Implemented dynamic license field handling with conditional form fields based on license type
  • Added validation and tooltip support for form fields
  • Created field definitions for related people and organizations (author, publisher, funder)
  • Implemented structure fields for managing dataset parts
packages/ecc-client-elixir-ro-crate/src/components/about/about.ts
Component demo and integration setup
  • Created demo pages for testing the component
  • Set up component registration for web components
  • Added TypeScript declarations for HTML element interface
packages/ecc-client-elixir-ro-crate/demo/about/index.html
packages/ecc-client-elixir-ro-crate/demo/index.html
packages/ecc-client-elixir-ro-crate/src/components/about/index.ts

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@vercel
Copy link

vercel bot commented Oct 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
elixir-cloud-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 1, 2024 2:46pm

@changeset-bot
Copy link

changeset-bot bot commented Oct 24, 2024

⚠️ No Changeset found

Latest commit: 948a721

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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 error handling in _handleDataset() - the findIndex could fail silently if licence field is not found. Add appropriate error handling or user feedback.
  • There is significant duplication in the RelatedPeopleFields definitions for author/publisher/funder. Consider extracting common field patterns into reusable constants or utility functions.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@salihudickson
Copy link
Contributor

Great work @sivangbagri, just a few things:

  1. Is this tool geared toward a specific type of RO-Crates like Workflow Run RO-Crates? or is it more generic with more options for users that want to create something more specifc.

  2. The fields for the entities on the "Related People, Orgs & Works" tab don't correspond to the required fields for those entities, can you please cite the resource you used to get those fields, incase I am missing something.

  3. I think the layout needs to be changed, I think the tabs should correspond to the entity names or at least entities that can be grouped together. For example, the about tab should be the Dataset Entity tab, we can also have one for the metadata (that can be added in a separate PR).

  4. The type field for the Dataset Entity should not be modifiable, you can actually get rid of that field and set that field programmatically, as the value MUST be "Dataset".

@sivangbagri
Copy link
Contributor Author

Hii @Salihu Thanks for your review.

  1. Currently I am keeping it as simple RO-Crate dataset with basic elements but obviously it can be improved in future to meet any particular requirement.
  2. Thats a todo thing actually, they are not the final fields. Will work on that separately.
  3. Yeah I will improve that . 👍🏻
  4. Yeah we can do that as root entity must be dataset only, I kept it for consistency and user awareness.

@salihudickson
Copy link
Contributor

  1. Okay, that makes sense. I do think it will be a good idea to add options in the future.
  2. If you haven't already then you might want to create an issue for that.

@sivangbagri
Copy link
Contributor Author

@salihudickson Kindly checkout the new PR

@salihudickson
Copy link
Contributor

I'll review the other PR ASAP, however this one is just about ready to merge, if you can just take out the type field, and change the tab name to Dataset Entity.

@salihudickson
Copy link
Contributor

Also please resolve conflicts with main.

@salihudickson
Copy link
Contributor

Hey, Shivang. This looks good so far. I made some changes to the logic handling the conditional license fields. The former logic was pretty convoluted and would break under certain conditions.

then about the type field. Perhaps removing it completely could cause some confusion and make the user believe there's no type property. I am going to write an issue requesting support for readonly fields in the form component, we can then use that to signify that there is a type field present but it is uneditable.

@salihudickson salihudickson merged commit a664ba5 into elixir-cloud-aai:main Nov 1, 2024
2 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.

2 participants