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-link-resource-button): Add Link Resource Button (DEV-404) #645

Merged
merged 16 commits into from Feb 14, 2022

Conversation

mdelez
Copy link
Collaborator

@mdelez mdelez commented Jan 31, 2022

resolves DEV-404

…en clicking on add new link value button while creating a resource
@mdelez mdelez added the enhancement New feature or request label Jan 31, 2022
@mdelez mdelez self-assigned this Jan 31, 2022
@mdelez mdelez marked this pull request as ready for review Feb 10, 2022
@mdelez mdelez requested a review from kilchenmann as a code owner Feb 10, 2022
<form [formGroup]="propertiesForm" (ngSubmit)="onSubmit()">
<!-- upload file -->
<app-upload *ngIf="hasFileValue"
[parentForm]="propertiesForm"
[representation]="hasFileValue"
(fileInfo)="setFileValue($event)">
</app-upload>

<app-select-properties #selectProps
[ontologyInfo]="ontologyInfo"
[resourceClass]="resourceClass"
[properties]="properties"
[parentForm]="propertiesForm"
class="select-properties">
</app-select-properties>
<div class="form-panel form-action">
<span>
<button mat-button type="button" (click)="closeDialog.emit()">
{{ 'appLabels.form.action.cancel' | translate }}
</button>
</span>
<span class="fill-remaining-space"></span>
<span>
<button mat-raised-button type="submit" color="primary" class="form-submit" [disabled]="!propertiesForm.valid">
{{ 'appLabels.form.action.submit' | translate}}
</button>
</span>
</div>
</form>
Copy link
Contributor

@kilchenmann kilchenmann Feb 11, 2022

Choose a reason for hiding this comment

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

Is the create-link-resource.component the same as in the second step of resource-instance-form? Does it make sense to duplicate the code?

Copy link
Collaborator Author

@mdelez mdelez Feb 11, 2022

Choose a reason for hiding this comment

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

@kilchenmann they're quite similar but the resource-instance-form has a back button to go back to the first step and it redirects the user to the created resource after it's been created. resource-instance-form uses an array of ResourceClassDefinition whereas create-link-resource does not use an array. We could try to condense it all into one component but we might have a lot of ugly if statements to hide/show relevant parts of the component depending on what the user is doing. What are your thoughts?

Copy link
Contributor

@kilchenmann kilchenmann Feb 11, 2022

Choose a reason for hiding this comment

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

Thanks for your response. That's true. Sooner or later the resource-instance-form will anyway have only one form (without the first step) and the back button will not be needed anymore. I would appreciate it if we do not have too many of the same forms.

Copy link
Collaborator Author

@mdelez mdelez Feb 14, 2022

Choose a reason for hiding this comment

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

Should I refactor in this PR or another one?

Copy link
Contributor

@kilchenmann kilchenmann Feb 14, 2022

Choose a reason for hiding this comment

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

What do you prefer? I think it should be resolved in this PR... or what will be the simplest solution?

Copy link
Collaborator Author

@mdelez mdelez Feb 14, 2022

Choose a reason for hiding this comment

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

after speaking with André, we decided to do the refactor in another PR because the create-resource-instance form will change in the future

@kilchenmann kilchenmann self-requested a review Feb 14, 2022
@mdelez mdelez merged commit f762c3c into main Feb 14, 2022
13 checks passed
@mdelez mdelez deleted the wip/dev-404-add-link-value-button branch Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants