Skip to content
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

Feature/library rooms #658

Merged
merged 26 commits into from
Jun 21, 2023
Merged

Conversation

jlcrodrigues
Copy link
Contributor

Closes #627
Fetched the reserved library rooms (gabinetes) from the website and displayed them in the library page.
This uses changes from #618.
If anyone wants to test this, rooms can be reserved here.

To view the currently reserved rooms, I added a card to the library page. The remove button is only a placeholder for now. Functionality will be added later (#656).
Screenshot from 2022-12-11 11-21-07

Moreover, when there's no rooms to display:
image

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes new summary in whatsnew/whatsnew-pt-PT
  • Properly adds entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well structured code

@jlcrodrigues
Copy link
Contributor Author

This is now ready for review. Added tabs to the library page separating occupations and reservations. There's also a card for the home page.

Note that the remove button is not yet functional (#656 ). A way to create reservations will also be added later on (#657 ).

Screenshot_1676237136 Screenshot from 2023-02-12 21-26-06

@jlcrodrigues jlcrodrigues marked this pull request as ready for review February 12, 2023 21:50
@brunogomes30 brunogomes30 changed the base branch from develop to feature/library-reservations March 15, 2023 15:20
@jlcrodrigues jlcrodrigues marked this pull request as draft March 15, 2023 20:51
@jlcrodrigues jlcrodrigues marked this pull request as ready for review March 15, 2023 23:22
@jlcrodrigues
Copy link
Contributor Author

This is now switched to providers.

Note: I used two routes for the reservation and rooms. Therefore, two items will appear in the drawer.

uni/lib/model/providers/library_reservations_provider.dart Outdated Show resolved Hide resolved
Comment on lines 28 to 31
final TabController? tabController = DefaultTabController.of(context);
if (widget.startOnOccupation) {
tabController!.index = 1;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be part of the state of the LibraryPageViewState and not getBody method context?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant here is, should the final TabController? tabController be declared at LibraryPageViewState and not the getBody function?

uni/lib/view/library/library.dart Outdated Show resolved Hide resolved
uni/lib/view/library/widgets/library_occupation_tab.dart Outdated Show resolved Hide resolved
uni/lib/view/library/widgets/library_occupation_tab.dart Outdated Show resolved Hide resolved
uni/lib/view/library/widgets/library_reservations_tab.dart Outdated Show resolved Hide resolved
Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it's a great progress to UNI and a great feature that it's not so easy to implement, so congratulations on that! I just have some minor feedback below, to make the code more maintainable and clean.

uni/lib/view/library/widgets/library_reservations_tab.dart Outdated Show resolved Hide resolved
import 'package:uni/model/entities/time_utilities.dart';

class ReservationRow extends StatelessWidget {
final LibraryReservation? reservation;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the nullable type, since this can only be built if a reservation exists in library_reservations_tab.dart. And it makes the code more readable too and with less null checks, in this file.

uni/lib/view/library/widgets/library_reservations_tab.dart Outdated Show resolved Hide resolved
'''CREATE TABLE RESERVATION(
id INTEGER PRIMARY KEY AUTOINCREMENT,
room TEXT,
startDate INT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the startDate use the ISO 8601 standard instead of an unix epoch to improve database consistency between models, since Exams already use it and Schedules will use it.

@jlcrodrigues
Copy link
Contributor Author

Thank you for the reviews. Made changes according to your suggestions.

I didn't use RequestDependentWidgetBuilder because this requires the use of helper functions, which I avoided using. This class should be refactored when #737 is done.

@jlcrodrigues jlcrodrigues mentioned this pull request Apr 26, 2023
7 tasks
Copy link
Member

@thePeras thePeras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 🛫

Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@thePeras thePeras merged commit e582cf7 into feature/library-reservations Jun 21, 2023
3 checks passed
@thePeras thePeras deleted the feature/library-rooms branch June 21, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display reserved library rooms
3 participants