-
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
Note Details redesign #1752
Note Details redesign #1752
Conversation
Deployed to https://pr-1752.aam-digital.net/ |
35ce62a
to
c6ed3be
Compare
afa0f78
to
116b0f3
Compare
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.
code looks good 👍
I'll test UX and functional properly in the last PR
- we are now completely loosing the confirmation dialog on closing a popup form but this was partly missing before, so maybe a separate topic (--> Unsaved details forms not warning before navigating away #1424)
@@ -302,7 +301,7 @@ export class Entity { | |||
*/ | |||
public copy(generateNewId: boolean = false): this { | |||
const other = new (this.getConstructor())(this._id); | |||
Object.assign(other, cloneDeep(this)); | |||
Object.assign(other, this); |
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.
I think we added the deep clone for a reason to make sure references/arrays are not reused?
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 is a problem with that approach.
When we do a deep copy, we might make a "too deep" copy: E.g. the configurable enum values cannot be matched anymore because in the basic autocomplete component we use object equality but due to the deep copy this does not work anymore.
So I suggest that instead the subclasses take care of creating a proper copy that is as deep as it needs to be (this is anyway what we are already doing).
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.
Sounds slightly risky to rely on the sub-classes to remember this, but I understand the point.
Isn't this impossible to cover config-defined array properties then?
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.
Good point but then we have a slightly bigger topic to cover, I moved this to #1805
...-dev-project/attendance/activity-attendance-section/activity-attendance-section.component.ts
Show resolved
Hide resolved
src/app/child-dev-project/attendance/edit-attendance/edit-attendance.component.ts
Outdated
Show resolved
Hide resolved
src/app/child-dev-project/attendance/edit-attendance/edit-attendance.component.ts
Show resolved
Hide resolved
src/app/child-dev-project/notes/note-details/note-details.component.spec.ts
Show resolved
Hide resolved
Common popup buttons
# Conflicts: # src/app/child-dev-project/attendance/activity-attendance-section/activity-attendance-section.component.ts # src/app/child-dev-project/attendance/attendance-details/attendance-details.component.html # src/app/child-dev-project/attendance/attendance-details/attendance-details.component.ts # src/app/child-dev-project/children/child-block/child-block.component.ts # src/app/child-dev-project/children/child-details/grouped-child-attendance/grouped-child-attendance.component.ts # src/app/child-dev-project/children/children-list/bmi-block/bmi-block.component.spec.ts # src/app/child-dev-project/children/children-list/bmi-block/bmi-block.component.ts # src/app/child-dev-project/children/children-list/recent-attendance-blocks/recent-attendance-blocks.component.spec.ts # src/app/child-dev-project/children/children-list/recent-attendance-blocks/recent-attendance-blocks.component.ts # src/app/child-dev-project/children/dashboard-widgets/entity-count-dashboard/entity-count-dashboard.component.ts # src/app/child-dev-project/notes/notes-related-to-entity/notes-related-to-entity.component.ts # src/app/child-dev-project/schools/activities-overview/activities-overview.component.ts # src/app/child-dev-project/schools/child-school-overview/child-school-overview.component.spec.ts # src/app/child-dev-project/schools/child-school-overview/child-school-overview.component.ts # src/app/core/entity-components/entity-details/form/form.component.spec.ts # src/app/core/entity-components/entity-details/form/form.component.ts # src/app/core/entity-components/entity-details/related-entities/related-entities.component.spec.ts # src/app/core/entity-components/entity-details/related-entities/related-entities.component.ts # src/app/core/entity-components/entity-select/edit-entity-array/edit-entity-array.component.ts # src/app/core/entity-components/entity-utils/view-components/display-date/display-date.component.ts # src/app/core/form-dialog/form-dialog.service.spec.ts # src/app/core/form-dialog/form-dialog.service.ts # src/app/features/matching-entities/matching-entities/matching-entities.component.spec.ts # src/app/features/matching-entities/matching-entities/matching-entities.component.ts # src/app/features/todos/todos-related-to-entity/todos-related-to-entity.component.ts
and improve its layout
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.
pushed some small bug fixes and UI improvements. From my review and testing this looks good to go now! :-)
Co-authored-by: Sebastian <sebastian.leidig@gmail.com>
Kudos, SonarCloud Quality Gate passed! 2 Bugs No Coverage information |
🎉 This PR is included in version 3.20.0-master.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 3.20.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
see issue: #1083
Based on #1793 so review and merge this first.
Visible/Frontend Changes
Architectural/Backend Changes
ChildMeetingNoteAttendance
to also work with the form setup (maybe this can become a individual datatype rather than a combination ofchildren
andchildrenAttendance