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(list editor): Adds support for editing lists (DSP-741) #365

Merged
merged 13 commits into from Feb 4, 2021

Conversation

@mdelez
Copy link
Contributor

@mdelez mdelez commented Jan 20, 2021

resolves DSP-741

@mdelez mdelez force-pushed the wip/DSP-741-edit-list-item-attempt-2 branch from 2f66840 to ff312af Jan 28, 2021
@mdelez mdelez requested review from flavens and kilchenmann Feb 3, 2021
@mdelez
Copy link
Contributor Author

@mdelez mdelez commented Feb 3, 2021

@flavens @kilchenmann new edit feature can be found in the app in the list editor of a project and by hovering over a list node :)

Loading

@mdelez
Copy link
Contributor Author

@mdelez mdelez commented Feb 3, 2021

oh, also make sure you're using DSP-API 13.1.1!

Loading

color="primary"
[disabled]="saveButtonDisabled"
(click)="updateChildNode()">
Update
Copy link
Collaborator

@flavens flavens Feb 3, 2021

Choose a reason for hiding this comment

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

you used {{ 'appLabels.form.action.cancel' | translate }} line 30, should use the format {{ 'appLabels.form.action.update' | translate }} here as well?! It exists in the en.json file

Loading

Copy link
Contributor Author

@mdelez mdelez Feb 4, 2021

Choose a reason for hiding this comment

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

This was André's code that I moved around. I changed the text to use the translate pipe for 'update' in 2c88e7e.

Loading

);
}

buildForm(list: ListNodeInfo): void {
Copy link
Collaborator

@flavens flavens Feb 3, 2021

Choose a reason for hiding this comment

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

would it be possible to add some description for each method, and possibly for tricky code lines?

Loading

Copy link
Contributor Author

@mdelez mdelez Feb 4, 2021

Choose a reason for hiding this comment

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

added in 2c88e7e

Loading

@mdelez mdelez requested a review from flavens Feb 4, 2021
Copy link
Collaborator

@flavens flavens left a comment

Would it be possible to get some more method description? 🙈

Loading

@@ -86,51 +114,40 @@ export class ListItemFormComponent implements OnInit {
}
}

submitData() {
createChildNode() {
Copy link
Collaborator

@flavens flavens Feb 4, 2021

Choose a reason for hiding this comment

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

I will be picky: would it be possible to add in this file as well some method description?

Loading

Copy link
Contributor Author

@mdelez mdelez Feb 4, 2021

Choose a reason for hiding this comment

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

I tried my best as I don't fully understand some of the methods either 😅 added in be62277

Loading

@@ -54,7 +55,6 @@ export class ListItemComponent implements OnInit {
}
);
}

}

showChildren(id: string): boolean {
Copy link
Collaborator

@flavens flavens Feb 4, 2021

Choose a reason for hiding this comment

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

I will be picky: would it be possible to add in this file as well some method description? even though it is not from you

Loading

Copy link
Contributor Author

@mdelez mdelez Feb 4, 2021

Choose a reason for hiding this comment

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

added in be62277

Loading

@mdelez mdelez requested a review from flavens Feb 4, 2021
flavens
flavens approved these changes Feb 4, 2021
@mdelez mdelez merged commit 5b6ee4b into main Feb 4, 2021
4 checks passed
Loading
@mdelez mdelez deleted the wip/DSP-741-edit-list-item-attempt-2 branch Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants