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

feat: form for creating drs object #344

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

psankhe28
Copy link
Contributor

@psankhe28 psankhe28 commented Aug 11, 2024

Description

It adds a form for creating drs object

Fixes #(issue)
#336

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

Signed-off-by: Pratiksha Sankhe <[email protected]>
Copy link

changeset-bot bot commented Aug 11, 2024

⚠️ No Changeset found

Latest commit: cf3810f

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

vercel bot commented Aug 11, 2024

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

Name Status Preview Comments Updated (UTC)
elixir-cloud-components ❌ Failed (Inspect) Aug 19, 2024 2:50pm

Copy link
Contributor

sourcery-ai bot commented Aug 11, 2024

Reviewer's Guide by Sourcery

This pull request introduces a new form component for creating DRS objects. The component is implemented using LitElement and includes form fields, submission logic, and error handling. Additionally, a React wrapper for the component is provided, along with demo HTML files to showcase its usage.

File-Level Changes

Files Changes
packages/ecc-client-elixir-drs-filer/src/components/create-object/create-object.ts
packages/ecc-client-elixir-drs-filer/src/components/create-object/index.ts
packages/ecc-client-elixir-drs-filer/src/components/create-object/create-object.styles.ts
Implemented the create-object component, including its styles and export logic.
packages/ecc-client-elixir-drs-filer/demo/create-object/index.html
packages/ecc-client-elixir-drs-filer/demo/index.html
Created demo HTML files to showcase the create-object component.
packages/ecc-client-elixir-drs-filer/src/react/ecc-client-elixir-drs-create-object/index.ts
packages/ecc-client-elixir-drs-filer/src/react/index.ts
Created and exported a React wrapper for the create-object component.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

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 @psankhe28 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider modularizing the create-object.ts file further. The form field definitions could be moved to a separate configuration file to improve readability and maintainability.
  • Add unit tests for the new component to ensure its functionality and make future maintenance easier.
  • Please update the user-facing documentation to describe the new component and its usage, as indicated in the unchecked item in your PR checklist.
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 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 to tell me if it was helpful.

}

// form submit method
private async _submitForm(form: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider breaking down the _submitForm method into smaller functions

The _submitForm method is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions to improve readability and maintainability.

private async _submitForm(form: any) {
  const formData = this._extractFormData(form);
  const validatedData = this._validateFormData(formData);
  await this._processAndSendData(validatedData);
}

private _extractFormData(form: any): any {
  // Extract form data logic
}

private _validateFormData(data: any): any {
  // Validate form data logic
}

private async _processAndSendData(data: any): Promise<void> {
  // Process and send data logic
}

}

// Process alias data
private _processAlias = (value: Array<{ alias: string }>) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider using Array.map for a more declarative approach

The _processAlias method could be simplified using Array.map for a more declarative and idiomatic approach. For example: private _processAlias = (value: Array<{ alias: string }>) => value.map(({alias}) => alias);

Suggested change
private _processAlias = (value: Array<{ alias: string }>) =>
private _processAlias = (value: Array<{ alias: string }>) => value.map(({alias}) => alias);

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.

1 participant