-
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 additions #386
Note additions #386
Conversation
…ad of just one and added a notes service to provide the notes-data throughout the app
…at are part of a meeting
…odel for the children in a group-note
…than those that were present + fixed a bug where a note would be in the note component multiple times after saving
…than those that were present
# Conflicts: # src/app/children/children.service.spec.ts # src/app/children/notes/note-details/note-details.component.html # src/app/children/notes/note-details/note-details.component.scss # src/app/children/notes/notes-manager/notes-manager.component.ts # src/app/session/connection-state.enum.ts # src/app/session/database.service.provider.ts # src/app/session/local-session.ts # src/app/session/login-state.enum.ts # src/app/session/mock-session.service.ts # src/app/session/remote-session.ts # src/app/session/session.service.provider.ts # src/app/session/session.service.ts # src/app/session/sync-state.enum.ts # src/app/session/synced-session.service.spec.ts # src/app/session/synced-session.service.ts # src/app/session/util/state-handler.spec.ts # src/app/session/util/state-handler.ts
sorry for the merge conflicts with the master branch, this will be mostly due to the moving of files into a new folder structure (#394). Is this otherwise complete and ready for review and merge? |
Concerning features and other changes, this is done. |
…hools # Conflicts: # src/app/child-dev-project/children/child-details/child-details.component.spec.ts # src/app/child-dev-project/children/children.module.ts # src/app/child-dev-project/children/model/childSchoolRelation.ts # src/app/child-dev-project/children/view-schools-component/edit-school-dialog/edit-school-dialog.component.html # src/app/child-dev-project/children/view-schools-component/edit-school-dialog/edit-school-dialog.component.spec.ts # src/app/child-dev-project/children/view-schools-component/edit-school-dialog/edit-school-dialog.component.ts # src/app/child-dev-project/children/view-schools-component/view-schools.component.html # src/app/child-dev-project/children/view-schools-component/view-schools.component.scss # src/app/child-dev-project/children/view-schools-component/view-schools.component.ts # src/app/child-dev-project/notes/note-details/note-details.component.html # src/app/child-dev-project/notes/notes-component/notes.component.spec.ts # src/app/child-dev-project/schools/model/schoolWithRelation.ts # src/app/core/entity/schema/entity-schema.service.ts # src/app/core/ui-helper/entity-subrecord/entity-subrecord.component.html # src/app/core/ui-helper/entity-subrecord/entity-subrecord.component.ts
# Conflicts: # src/app/child-dev-project/children/child-details/child-details.component.spec.ts # src/app/child-dev-project/children/children.module.ts # src/app/child-dev-project/notes/note-details/note-details.component.html # src/app/child-dev-project/notes/notes-component/notes.component.spec.ts # src/app/core/entity/schema/entity-schema.service.ts # src/app/core/ui-helper/entity-subrecord/entity-subrecord.component.html # src/app/core/ui-helper/entity-subrecord/entity-subrecord.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.
This is great functionality (you seem to have thought about every related aspect!) and the code looks really good already 👍
Please have a look at my few remaining comments and questions.
Functional testing went well. However, I see a few points where we could still polish for UX:
- I think the child-select and the presence list elements should be directly next to each other (maybe move the child-select down, above the presence list). That will make it easier for new users to understand the connection.
- How to switch children between present/absent is not obvious for non-technical users. We could try whether a toggle [Present|Absent] instead of the arrow makes it more transparent. Or maybe you have other ideas?
- Meeting notes are very highlighted in the Notes section of the Child Details view currently, because most other Notes just have white background. I'm not sure how to improve that easily however and don't think this is a blocker. But maybe you have any ideas?
@@ -57,7 +59,7 @@ export class ChildSelectComponent implements OnInit { | |||
selectChild(child: Child, suppressChangeEvent = false) { | |||
this.selectedChildren.push(child); | |||
if (!suppressChangeEvent) { | |||
this.valueAsIdsChange.emit(this.selectedChildren.map(c => c.getId())); |
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.
the EventEmitter named valueAsIdsChange
is important to allow two-way databinding (details) on the property valueAsIds
(if you can think of a better name, please suggest a rename!). We should keep the emit() for that as well and have both outputs in parallel, I think.
@@ -4,7 +4,7 @@ import { Child } from './model/child'; | |||
import { EntityMapperService } from '../../core/entity/entity-mapper.service'; | |||
import { AttendanceMonth } from '../attendance/model/attendance-month'; | |||
import { Database } from '../../core/database/database'; | |||
import { Note } from '../notes/model/note'; | |||
import { NoteModel } from '../notes/note.model'; |
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.
Any particular reason to rename the Note
class to NoteModel
? I think we should keep this consistent for all Entity classes and personally favour the short Note
but we can put this up for discussion
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 have done this to distinguish the new NoteModel
from the old Note
during refactoring from a component to a module. I kept the change to make it clear that this was a model. Since I agree that this is no longer needed, I will revert the changes
this.childrenService.getNotesOfChild(this.childId) | ||
.subscribe(results => this.records = results.sort((a, b) => { | ||
return (b.date ? b.date.valueOf() : 0) - (a.date ? a.date.valueOf() : 0); } )); | ||
this.entityMapperService.loadType<NoteModel>(NoteModel).then(notes => { |
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.
Loading every note and doing in-memory filtering may become a performance issue. ChildrenService.getNotesOfChild()
uses a pouchdb database index and should (in theory) be the better choice.
Did you have any particular issues that pushed you to change this here?
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 had an issue with this but could resolve this in another way. Until now I thought that these two ways of getting the data were equal in performance. If that's not the case, I will revert these changes as well
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.
Looking at the code right now, the issue was not resolved. ChildrenService
filters if the children-array of the Note contains the child-id (which is never true since the Array is of type AttendanceModel
). Is it possible to change the database quarry to check if any of it's AttendanceModel
s contains the id?
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.
For the PouchDb/CouchDB based Database queries should be more efficient, the database then only loads the required documents and returns them (and hopefully the developers of pouchdb have figured out a way to do that reasonably efficient).
The query index is also created in the ChildrenService:
ndb-core/src/app/child-dev-project/children/children.service.ts
Lines 229 to 243 in 134d808
private createNotesIndex(): Promise<any> { | |
const designDoc = { | |
_id: '_design/notes_index', | |
views: { | |
by_child: { | |
map: '(doc) => { ' + | |
'if (!doc._id.startsWith("' + Note.ENTITY_TYPE + '")) return;' + | |
'doc.children.forEach(childId => emit(childId)); ' + | |
'}', | |
}, | |
}, | |
}; | |
return this.db.saveDatabaseIndex(designDoc); | |
} |
Beware, unfortunately the function has to be a string containing the javascript function (to prevent the build optimization from messing this up). You can also refer to the PouchDB guide about how those query functions work: https://pouchdb.com/guides/queries.html
Basically you'll just have to change that function to emit()
the childId from its new nested place.
@@ -0,0 +1,21 @@ | |||
|
|||
export class AttendanceModel { |
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.
This sounds very similar to our AttendanceMonth
class and the (completely separate) system for keeping students' school attendance. Maybe MeetingNoteAttendance
or something like that can make its purpose more clear?
<!--Small screen: display the notes in tabs--> | ||
<div *ngIf="entity.isMeeting() && smallScreen" fxLayout="row wrap" fxLayoutGap="20px"> | ||
|
||
<mat-tab-group mat-align-tabs="center" fxFlex> | ||
<mat-tab label="Present" class="padding-container" > | ||
<app-child-presence-list [note]="entity" [recordForm]="recordForm" [present]="true" fxFlex></app-child-presence-list> | ||
</mat-tab> | ||
<mat-tab label="Absent" class="padding-container" > | ||
<app-child-presence-list [note]="entity" [recordForm]="recordForm" [present]="false" fxFlex></app-child-presence-list> | ||
</mat-tab> | ||
</mat-tab-group> | ||
|
||
</div> | ||
|
||
<!--Big screen: display the notes as two lists--> | ||
<div *ngIf="entity.isMeeting() && !smallScreen" fxLayout="row wrap" fxLayoutGap="20px"> | ||
<app-child-presence-list [note]="entity" | ||
[recordForm]="recordForm" | ||
[present]="true" | ||
[label]="'Present'" | ||
class="padding-bottom" | ||
fxFlex> | ||
</app-child-presence-list> | ||
<app-child-presence-list [note]="entity" | ||
[recordForm]="recordForm" | ||
[present]="false" | ||
[label]="'Absent'" | ||
class="padding-bottom" | ||
fxFlex> | ||
</app-child-presence-list> | ||
</div> |
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.
Let's extract this into a separate component as well so the responsive version can be easily re-used (and the template here will be shorter and more manageable). Possibly such a new component could also include the app-child-select
component above and be somewhat of an alternative to the simple app-child-select
…hools # Conflicts: # src/app/core/admin/export-data/export-data.component.html
closing this as PR #431 is built upon this branch |
see issue: #221, #381, #145
Visible/Frontend Changes
notes-list
componentNote-Manager
-component uses a paginatorArchitectural/Backend Changes
Note
now is now it's own module and is no longer a sub-component of theChildrenModule
NotesService
has been implemented to fetch resources from theEntityMapper
and distribute updates throughout every listening componentEntitySubrecord
andChildSelect
-components to enable these new features