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: Add components #29

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

Conversation

MokumJ
Copy link

@MokumJ MokumJ commented Aug 16, 2022

  • CRUD component
  • add/update component fields to model/component

@MokumJ
Copy link
Author

MokumJ commented Aug 16, 2022

I went for an approach where if you give multiple components to the addComponentField it creates under the hood a UnionComponentField if you add just one component it creates a basic component. Not sure or this is an approach you like but felt convenient for me

@rajatsharma
Copy link
Member

Hey @MokumJ 👋🏼 Thanks for creating the PR. I am sorry it took us time to review this.

Would it be possible if we can have separate methods for Component and ComponentUnion field? Because there can be scenarios where we only want a single component in ComponentUnion field.

Also I was facing some issues with type of the addComponentField method. I believe we should type it like so

  addComponentField(
    field: Omit<
      CreateComponentFieldArgs,
      "modelApiId" | "parentApiId" | "componentApiIds"
    >
  ): Model;

I have added modelApiId and parentApiId to Omit because we can get it from this.args.apiId. Consequently, the type of addComponentUnionField would be

  addComponentUnionField(
    field: Omit<
      CreateComponentFieldArgs,
      "modelApiId" | "parentApiId" | "componentApiId"
    >
  ): Model;

I am removing componentApiId because we only need componentApiIds.

What do you think about this?

@MokumJ
Copy link
Author

MokumJ commented Sep 6, 2022

Hi @rajatsharma, glad to hear from you and great you are picking this up :) I agree with separating the component and ComponentUnion field. I guess its also more in line with previous use case of simplefield/unionfield. I see indeed those types should be omitted.

Are you picking up this points en add to the commit or should I have a look into it?

I am not sure when I would have time for this. Maybe next monday.

@MokumJ
Copy link
Author

MokumJ commented Sep 12, 2022

@rajatsharma
I updated the Pr to your suggestions. It seems adding a union component to a component field is not working. I think this is due to additions fixes that should be done to your management server as part of a batch migration service. Am I right?

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