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(canvas): Add a container for Pipes #1086

Merged
merged 1 commit into from
May 20, 2024
Merged

Conversation

shivamG640
Copy link
Contributor

Fixes: #1031

Description###

  • Loads container for Pipes.

  • Loads the Configuration form when a user clicks on the Pipe Container.

  • Manage the sync between source-code and form.

@shivamG640 shivamG640 force-pushed the Fix_1031 branch 2 times, most recently from 59091ad to 53672a9 Compare May 15, 2024 11:45
@shivamG640 shivamG640 marked this pull request as ready for review May 15, 2024 11:49
@@ -48,6 +60,10 @@ export class PipeVisualEntity implements BaseVisualCamelEntity {
getNodeLabel(path?: string): string {
if (!path) return '';

if (path === ROOT_PATH) {
return this.id;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't rather this be the name of the Pipe and fallback to the id if not present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are always assigning the pipe name as id so will there be any chance of id being undefined?
As far as I think, pipe name can be update either from the source-code or from the form, both of the cases are handled!

Case 1. When the pipe name gets updated form the Source-code.
Case 2. When the pipe name gets updated from the form

Copy link
Member

Choose a reason for hiding this comment

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

You're right @shivamG640 👍 , let's keep it as it is

packages/ui/src/utils/pipe-custom-schema.ts Outdated Show resolved Hide resolved
packages/ui/src/utils/pipe-custom-schema.ts Outdated Show resolved Hide resolved
packages/ui/src/utils/pipe-custom-schema.ts Outdated Show resolved Hide resolved
packages/ui/src/utils/pipe-custom-schema.ts Outdated Show resolved Hide resolved
@shivamG640
Copy link
Contributor Author

Hi, @lordrip
The latest changes are deployed here: https://shivamg640.github.io/kaoto/#/

@shivamG640 shivamG640 requested a review from lordrip May 17, 2024 21:56
@@ -70,14 +94,20 @@ export class PipeVisualEntity implements BaseVisualCamelEntity {
}

toJSON() {
return this.spec;
return this.pipe.spec;
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit strange, here we're just returning the spec section to be serialized, rather than the entire Pipe definition.

Copy link
Member

Choose a reason for hiding this comment

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

I see it now, is the Pipe resource who serializes the entire object here:

  toJSON(): PipeType {
    return this.pipe;
  }

Copy link
Member

@lordrip lordrip left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for taking care of it @shivamG640.

@lordrip
Copy link
Member

lordrip commented May 20, 2024

The stories for Pipe are now broken because of the new Pipe constructor. I'll take care.

image

@lordrip lordrip merged commit e70cfb6 into KaotoIO:main May 20, 2024
8 of 9 checks passed
lordrip added a commit to lordrip/kaoto that referenced this pull request May 20, 2024
After merging KaotoIO#1086, Storybook
needs to be updated to accomodate for the new `Pipe` constructor
lordrip added a commit that referenced this pull request May 20, 2024
After merging #1086, Storybook
needs to be updated to accomodate for the new `Pipe` constructor
@shivamG640
Copy link
Contributor Author

Thanks for adding the Stories fix @lordrip

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.

Add a container for Pipes
2 participants