-
Notifications
You must be signed in to change notification settings - Fork 22
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
Andrei mirica19/replace year week number #297
base: master
Are you sure you want to change the base?
Changes from 15 commits
5e85fb3
86ebbff
2bf465b
064d9f3
be030ab
7b5be1f
f33ccef
43c14bd
1d840f9
7e77048
b7a466d
a744eb4
e8f55f1
a63224d
15852c8
a56d7a7
c834c3d
5cfec27
bc1bf14
5f8c165
c10f563
63d5f1e
7420458
5d18e0a
e5dcc6a
41c9eca
79e8316
40ae3ef
f5e376c
2f3646d
e7b0e27
96bbac1
4235f8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import 'package:acs_upb_mobile/generated/l10n.dart'; | |
import 'package:acs_upb_mobile/navigation/routes.dart'; | ||
import 'package:acs_upb_mobile/pages/settings/service/request_provider.dart'; | ||
import 'package:acs_upb_mobile/pages/timetable/service/uni_event_provider.dart'; | ||
import 'package:acs_upb_mobile/pages/timetable/view/lead_header.dart'; | ||
import 'package:acs_upb_mobile/resources/locale_provider.dart'; | ||
import 'package:acs_upb_mobile/resources/utils.dart'; | ||
import 'package:acs_upb_mobile/widgets/icon_text.dart'; | ||
|
@@ -31,6 +32,7 @@ class _SettingsPageState extends State<SettingsPage> { | |
|
||
// String describing the level of editing permissions that the user has. | ||
String userPermissionString = ''; | ||
bool isSwitched = false; | ||
|
||
@override | ||
void initState() { | ||
|
@@ -116,9 +118,7 @@ class _SettingsPageState extends State<SettingsPage> { | |
), | ||
), | ||
Visibility( | ||
visible: authProvider.isAuthenticated && | ||
!authProvider.isAnonymous && | ||
authProvider.currentUserFromCache.isAdmin == true, | ||
visible: authProvider.currentUserFromCache.isAdmin == true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a broken merge, please revert the condition. |
||
child: ListTile( | ||
key: const Key('AdminPanel'), | ||
onTap: () => | ||
|
@@ -149,6 +149,23 @@ class _SettingsPageState extends State<SettingsPage> { | |
subtitle: Text(S.current.infoExportToGoogleCalendar), | ||
), | ||
), | ||
Column( | ||
children: [ | ||
SwitchPreference( | ||
'Academic week number ', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All user-facing strings should be localized: |
||
'Academic week number', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a nicer key, like we did above (e.g. |
||
onEnable: () { | ||
LeadHeader.academicWeekNumber = true; | ||
}, | ||
onDisable: () { | ||
LeadHeader.academicWeekNumber = false; | ||
}, | ||
defaultVal: false, | ||
desc: | ||
'show number of week in the semester instead of the calendar year', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should start with uppercase to match the other strings on the page |
||
), | ||
], | ||
), | ||
PreferenceTitle(S.current.labelFeedback), | ||
ListTile( | ||
key: const ValueKey('feedback_and_issues'), | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,123 @@ | ||||
import 'package:acs_upb_mobile/authentication/model/user.dart'; | ||||
import 'package:acs_upb_mobile/authentication/service/auth_provider.dart'; | ||||
import 'package:acs_upb_mobile/pages/classes/model/class.dart'; | ||||
import 'package:acs_upb_mobile/pages/classes/service/class_provider.dart'; | ||||
import 'package:acs_upb_mobile/pages/people/model/person.dart'; | ||||
import 'package:acs_upb_mobile/pages/people/service/person_provider.dart'; | ||||
import 'package:acs_upb_mobile/pages/timetable/model/academic_calendar.dart'; | ||||
import 'package:acs_upb_mobile/pages/timetable/service/uni_event_provider.dart'; | ||||
import 'package:black_hole_flutter/black_hole_flutter.dart'; | ||||
import 'package:flutter/material.dart'; | ||||
import 'package:time_machine/time_machine.dart'; | ||||
|
||||
// ignore: implementation_imports | ||||
import 'package:timetable/src/theme.dart'; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||
|
||||
// ignore: implementation_imports | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment isn't necessary, this isn't an implementation import. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You removed the comment above on line 12, which was actually necessary instead of removing this comment. Please pay attention. |
||||
import 'package:provider/provider.dart'; | ||||
|
||||
class LeadHeader extends StatefulWidget { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe rename to TimetableLeadingHeader for a more descriptive name? |
||||
const LeadHeader(this.date, {Key key}) : super(key: key); | ||||
final LocalDate date; | ||||
static var academicWeekNumber = false; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Static vars are a no-no, see my comment on academic_calendar.dart. |
||||
|
||||
@override | ||||
_LeadHeaderState createState() => _LeadHeaderState(); | ||||
} | ||||
|
||||
class _LeadHeaderState extends State<LeadHeader> { | ||||
List<ClassHeader> classHeaders = []; | ||||
List<Person> classTeachers = []; | ||||
User user; | ||||
AcademicCalendar calendar; | ||||
Map<String, AcademicCalendar> calendars = {}; | ||||
int academicWeek; | ||||
Set<int> holidayWeeks = {}; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Except for maybe |
||||
|
||||
@override | ||||
Widget build(BuildContext context) { | ||||
final theme = context.theme; | ||||
final timetableTheme = context.timetableTheme; | ||||
final defaultBackgroundColor = theme.contrastColor.withOpacity(0.12); | ||||
final textStyle = timetableTheme?.weekIndicatorTextStyle ?? | ||||
TextStyle( | ||||
color: defaultBackgroundColor | ||||
.alphaBlendOn(theme.scaffoldBackgroundColor) | ||||
.mediumEmphasisOnColor, | ||||
fontSize: 14); | ||||
|
||||
return Container( | ||||
child: Center( | ||||
child: Container( | ||||
width: 27, | ||||
height: 20, | ||||
Comment on lines
+52
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be the reason why we're getting an overflow in the tests. We should generally avoid fixed sizes for things, since they'll look different depending on screen size (and may overflow on smaller screens). Can't we use exactly Jonas' implementation and just replace the |
||||
decoration: BoxDecoration( | ||||
borderRadius: BorderRadius.circular(2), | ||||
color: defaultBackgroundColor, | ||||
border: Border.all(color: defaultBackgroundColor, width: 1)), | ||||
child: Text( | ||||
getWeekNumber(), | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move the setting check here (see my comment below, I'd like to move the logic of fetching the week to the calendar itself, and the calendar shouldn't know about settings). |
||||
style: textStyle, | ||||
textAlign: TextAlign.center, | ||||
), | ||||
), | ||||
), | ||||
); | ||||
} | ||||
|
||||
String getWeekNumber() { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this to the calendar class directly, maybe make it a getter? Like |
||||
if (calendar != null) { | ||||
final List<int> nonHolidayWeeks = calendar.nonHolidayWeeks.toList(); | ||||
final DateInterval winterSession = DateInterval( | ||||
calendar.exams.first.startDate, calendar.exams.first.endDate); | ||||
final DateInterval summerSession = DateInterval( | ||||
calendar.exams.last.startDate, calendar.exams.last.endDate); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may think you know what the exam sessions are, but the abstraction is there for a reason. Remember that we also have restanțe & sesiunea de diplomă for instance, we don't want variables for every one of them, that's why we're using a list of |
||||
for (var i = 1; i < 53; i++) { | ||||
if (!nonHolidayWeeks.contains(i)) holidayWeeks.add(i); | ||||
} | ||||
final week = | ||||
((widget.date.dayOfYear - widget.date.dayOfWeek.value + 10) / 7) | ||||
.floor(); | ||||
if (LeadHeader.academicWeekNumber == false) { | ||||
return week.toString(); | ||||
} else { | ||||
if (!nonHolidayWeeks.contains(week)) { | ||||
if (winterSession.contains(widget.date) || | ||||
summerSession.contains(widget.date)) { | ||||
return 'S'; | ||||
} else { | ||||
return 'H'; | ||||
} | ||||
} else { | ||||
return (nonHolidayWeeks.indexOf(week) + 1).toString(); | ||||
} | ||||
} | ||||
} else { | ||||
return '0'; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than 0, let's just return an empty string. |
||||
} | ||||
} | ||||
|
||||
@override | ||||
void initState() { | ||||
if (!mounted) { | ||||
return; | ||||
} | ||||
Comment on lines
+72
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks unnecessary. |
||||
super.initState(); | ||||
user = | ||||
Provider.of<AuthProvider>(context, listen: false).currentUserFromCache; | ||||
Provider.of<ClassProvider>(context, listen: false) | ||||
.fetchClassHeaders(uid: user.uid) | ||||
.then((headers) => setState(() => classHeaders = headers)); | ||||
Provider.of<PersonProvider>(context, listen: false) | ||||
.fetchPeople() | ||||
.then((teachers) => setState(() => classTeachers = teachers)); | ||||
Comment on lines
+76
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these necessary? |
||||
Provider.of<UniEventProvider>(context, listen: false) | ||||
.fetchCalendars() | ||||
.then((calendars) { | ||||
setState(() { | ||||
this.calendars = calendars; | ||||
calendar = calendars.values.last; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't work, what if we scroll to the previous year/semester? We have some existing logic for this in the calendar selection dropdown when adding an event: Let's do the following:
for (final semester in semesters) {
if (date.isBeforeOrDuring(semester)) {
// semester.id is represented as "semesterN", where "semester0" is the first semester
return 1 + int.tryParse(semester.id[semester.id.length - 1]);
}
}
return -1;
final LocalDate date = widget.initialEvent.start.calendarDate ?? LocalDate.today();
calendars.forEach((c) {
final semester = c.semesterForDate(date);
if (semester != -1) {
selectedCalendar = c;
selectedSemester = semester;
return;
}
});
(Keep in mind I didn't actually compile this code, so try to understand the logic and make changes where necessary) |
||||
}); | ||||
}); | ||||
} | ||||
} |
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.
?