-
Notifications
You must be signed in to change notification settings - Fork 21
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
Refactoring: entities-table / entity-subrecord #2142
Conversation
Deployed to https://pr-2142.aam-digital.net/ |
src/app/core/common-components/entities-table/list-paginator/list-paginator.component.scss
Outdated
Show resolved
Hide resolved
…ist-paginator.component.scss
...components/entities-table/entity-inline-edit-actions/entity-inline-edit-actions.component.ts
Show resolved
Hide resolved
…it-actions/entity-inline-edit-actions.component.ts
showEntity(entity: T) { | ||
switch (this.clickMode) { | ||
case "popup": | ||
this.formDialog.openFormPopup(entity, this.columnsToDisplay); // TODO this.formDialog.openFormPopup(entity, this._columns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not correctly support the following flags yet:
- hideFromTable?: boolean;
- hideFromForm?: boolean;
- forTable?: boolean;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--> discuss: where + how do we need these concepts; let's try to simplify and clearly specify this before re-adding the logic.
(e.g. todos-related-to-entity use the process)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good refactoring. Most comments are quite minor.
On functionality testing I discovered some bugs:
- trying to inline-edit a note (e.g. in child details) always opens the popup when clicking on a from field
- saving after inline editing does not reset the unsaved changes state (
UnsavedChangesService
) so the app still thinks there are unsaved changes - Editing and saving in a popup (note details or row details) does not update the value in the table after closed
- Readonly function is not shown in popup (e.g. health check BMI)
- More properties are shown in popup (e.g. health check also shows child id form)
src/app/child-dev-project/notes/notes-manager/notes-manager.component.ts
Outdated
Show resolved
Hide resolved
src/app/child-dev-project/schools/child-school-overview/child-school-overview.component.ts
Outdated
Show resolved
Hide resolved
src/app/child-dev-project/schools/child-school-overview/child-school-overview.component.ts
Outdated
Show resolved
Hide resolved
src/app/core/common-components/entities-table/entities-table.component.html
Outdated
Show resolved
Hide resolved
src/app/core/common-components/entities-table/entities-table.component.ts
Outdated
Show resolved
Hide resolved
src/app/core/common-components/entities-table/entities-table.component.ts
Outdated
Show resolved
Hide resolved
src/app/core/common-components/entities-table/entities-table.component.ts
Show resolved
Hide resolved
src/app/core/entity-details/related-entities/related-entities.component.ts
Outdated
Show resolved
Hide resolved
src/app/core/entity-details/related-entities/related-entities.component.ts
Outdated
Show resolved
Hide resolved
...p/core/entity-details/related-time-period-entities/related-time-period-entities.component.ts
Outdated
Show resolved
Hide resolved
… is undefined fixes #2172
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to all work well now. There are just two more minor things I discovered.
- Inline editing "cancel" does not correctly reset
unsavedChanges
state - The Task list does by default not show the archived tasks anymore. This results in the weird behavior that when activating the "completed" filter no tasks are shown until the "include archived" is also activated. This is probably something that most users will not figure out.
# Conflicts: # src/app/core/entity-details/related-entities/related-entities.component.spec.ts # src/app/core/entity-details/related-entities/related-entities.component.ts
# Conflicts: # src/app/core/entity-details/related-entities/related-entities.component.spec.ts # src/app/core/entity-details/related-entities/related-entities.component.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for also implementing the sorting automation.
🎉 This PR is included in version 3.29.0-master.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 3.29.0-master.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 3.29.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Co-authored-by: Simon <simon@aam-digital.com>
Open TODOs
potential further refactoring