-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add teachers to the timetable #1629
Conversation
Visit the preview URL for this PR (updated for commit 4f1e734): https://sharezone-website-dev--pr1629-add-teacher-3rraqs9a.web.app (expires Sun, 19 May 2024 07:46:37 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 372b0431a96247f908d9a97d5d865de1c8b3b04e |
Visit the preview URL for this PR (updated for commit 4f1e734): https://sharezone-console-dev--pr1629-add-teacher-63yxgzkn.web.app (expires Sun, 19 May 2024 07:46:24 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 471536afe3f6ec4895d9ea75513730b515d17eb6 |
Visit the preview URL for this PR (updated for commit 4f1e734): https://sharezone-test--pr1629-add-teacher-d319lxmr.web.app (expires Sun, 19 May 2024 07:47:30 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 4cb3ae61e1e018abfd9841fd3239f5b49ccc034b |
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 should first change Lesson.teacher
to be a Set/List of teacher (names) or a Map of objects where we could add more fields later. I mean its definitely possible that a lesson has several teachers. We could use String? get teacher => teachers.first
in the DTO at first so we can merge the feature with the current behavior and add support for several teachers in the future if we want. WDYT?
final teachers = | ||
lessons.where((l) => l.teacher != null).map((l) => l.teacher!); |
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.
Nit: Maybe a bit cleaner:
final teachers = | |
lessons.where((l) => l.teacher != null).map((l) => l.teacher!); | |
final teachers = lessons.map((l) => l.teacher).nonNulls; |
app/lib/timetable/timetable_edit/lesson/timetable_lesson_edit_page.dart
Outdated
Show resolved
Hide resolved
Idea: Split teachers at "," |
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 settled on using just the teacher
field for now.
Description
This pull request adds the Sharezone Plus feature to add teachers to the timetable.
Demo
Screen.Recording.2024-05-09.at.11.57.50.mov
Changed the lesson sheet to a dialog:
Reason: In the sheet you need to scroll to see the substitutions tiles and it's not easy to adjust the size of the sheet (depending on the screen size). A dialog is here the better option.
Users can also add the teacher when adding a lesson:
Sharezone Plus page:
Related Tickets
Closes #1608
Opens #1628