StudyRoom Feature #49

mammuth · 2016-12-18T14:47:28Z

I implemented the basic functionality of the Study Rooms Feature. It's linked in the More-View, fetches the rooms, and displays them in a sorted manner

Big ToDo's:

Decide on how to handle groups (sections vs. Picker as for movies vs. ?) (Right now there are only 3 groups: "Bibliothek Mathe/Informatik", "Magistrale Mathe/Informatik", "Garching-Hochbrück (EG)"
Link to Room Maps on click (of the whole row?)
Pull To Refresh (?)
Make layout "better" (I just moved the labels around to have "something", let's add some constraints)
Hate me for my color choices (Optional)

@mathiasquintero it'd be great if you could check this out and tell me whether I heavily ignored the wished patterns and concepts 😊 Also I'm pretty sure there is room for small improvements at many places.

The API request returns a JSON with rooms and groups in it. I'm not quite sure how to handle this, currently our TumDataReceiver is made to receive [DataElement]. Same for the Handlers. Any views on that?

I therefore extended The RoomFinder VC to handle StudyRooms as well.

As a possible downside the fetched rooms are not cached, when leaving and reentering the study rooms view. However this probably makes sense and should have been this way anyway.

kordianbruck · 2016-12-18T23:10:11Z

Please make sure, that any field might be empty at any time, so check for null/empty values.

Heres a Angular2 Codesnippet showing the current configuration for the website:

<div [ngClass]="{free:e.status == 'frei', full:e.status == 'belegt', unkown:e.status == 'unbekannt'}">
    <h4>
        {{e.raum_name}}
        <span *ngIf="e.status == 'belegt'">noch {{e.belegung_bis | amTimeAgo:true}}</span>
    </h4>
    <a href="https://campus.tum.de/tumonline/ris.einzelraum?raumkey={{e.raum_nr}}">{{e.raum_code}}</a>
    <span *ngIf="e.status == 'belegt'">
        | {{e.belegung_durch}}<br/>
        <a href="https://campus.tum.de/tumonline/wbKalender.wbRessource?pResNr={{e.res_nr}}{{calenderDisplayMode}}">
            Belegt <span *ngIf="e.belegung_ab">von {{e.belegung_ab | amCalendar }}</span> bis {{e.belegung_bis | amCalendar}}
        </a>
    </span>
</div>

nerdsupremacist · 2016-12-19T02:31:54Z

Looks great. I would want to work on the UI a bit more. All green and red cells don't really go with the current design motive. Perhaps move it to the a small indicator view and the font color. If it's cool with you I'll do it ;)

nerdsupremacist · 2016-12-19T02:35:15Z

@mammuth How are there groups and rooms. In the same array? Or are the rooms in the groups?

mammuth · 2016-12-19T07:10:42Z

There are two arrays. A RoomGroup has an array of roomNumbers. Currently the manager is calling the handler twice (once with groups as argument and once with rooms), which doesn't feel clean but the only other way I could thought of was to don't use the given protocols (Data manager and receiver protocols), since they force the handler in the manager to take a [DataElement]. Sure, feel free to improve the UI! Mathias Quintero <notifications@github.com> schrieb am Mo., 19. Dez. 2016, 03:35:

…

@mammuth <https://github.com/mammuth> How are there groups and rooms. In the same array? Or are the rooms in the groups? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC-gmsW1emHZwp40fWLitqDCHTsaQLmLks5rJe1jgaJpZM4LQJSC> .

mammuth · 2017-03-03T10:47:19Z

Resolves #24

mammuth · 2017-03-31T11:25:38Z

So this is not merged yet because the design needs some overhauls.

Looks great. I would want to work on the UI a bit more. All green and red cells don't really go with the current design motive. Perhaps move it to the a small indicator view and the font color. If it's cool with you I'll do it ;)

@mathiasquintero are you on this? Otherwise I can try to implement your suggestion with transforming the visual indicator to the headline.

# Conflicts: # TUM Campus App.xcodeproj/project.pbxproj # TUM Campus App/More.storyboard # TUM Campus App/TumDataItems.swift

mammuth · 2017-04-02T06:43:32Z

Something like this?

The "FMI/ Bibliothek" is actually redundant, isn't it? Those three "locations" are stated in the title bar and can be changed there, so there is actually no need to repeat it in every row.

mammuth · 2017-04-02T07:00:12Z

I needed to do some merging with the More storyboard and the project file, hope I didn't break anything.

Ready to review @mathiasquintero . Also I didn't add any constraints yet.

Don’t know why, but the run configurations have not been properly set for the testing targets

kordianbruck · 2017-04-02T10:30:23Z

@mammuth I'd go for the second design for sure! Looks very nice!

nerdsupremacist · 2017-04-15T21:26:12Z

@mammuth please check that I didn't break anything during my merge. All the stuff that could go horribly wrong was in the pods xcodeproj so if it's broken do a pod install and it should be fixed :D

I'm not a machine I can't keep track of so many id's.
Did I mention that I hate xcodeproj files? Because I do

nerdsupremacist · 2017-04-15T21:29:18Z

fastlane/Deliverfile

+###################### Automatically generated ######################
+# Feel free to remove the following line if you use fastlane (which you should)
+
+app_identifier "de.tum.campusapp" # The bundle identifier of your app


What's this?

That's the configuration file for fastlane deliver. "de.tum.campusapp" is the bundle id we use at the published app if I'm right.

However, this file probably shouldn't be tracked in git, since it's personalized.

I think you have also the permission to add builds to iTunes Connect, right? Then you can initiate deliver by running "fastlane deliver" in the project root once.

We can keep track of the changelog, screenshots, store description etc. in the repo in the metadata (I think?) directory. We can then use fastlane deliver to publish those metadata to iTunes Connect or even submit new builds for review.

See https://github.com/fastlane/fastlane/tree/master/deliver for usage details.

Damn Fastlane is really fancy!

nerdsupremacist · 2017-04-15T21:29:49Z

TUM Campus App/TumStyleConstants.swift

please use lowerCamelCase

nerdsupremacist · 2017-04-15T21:32:38Z

TUM Campus App/StudyRoomsManager.swift

+                                if let data = response.result.value {
+                                    if let json = JSON(data).dictionary {
+                                        if let rooms = json["raeume"]?.array {
+                                            StudyRoomsManager.studyRooms.removeAll()


If we're already removeing everything and setting it again just write:

StudyRoomsManager.studyRooms = rooms.flatMap(StudyRoom.init)

Are my recent changes as you expected them to be?

Well then the loops are unnecesary. The flatMap does all the work for you

And you should set the Groups inside the other if. Not here, right?

since it is personal and doesn’t contain any meaningful settings (yet)

mammuth · 2017-04-16T20:00:38Z

Did I mention that I hate xcodeproj files? Because I do

I'm completely new to iOS, but this is probably the most insane thing I saw while learning it. I just don't get it why developers want to make settings in a fancy GUI which then get stored in an not-easy-to-understand binary-like insanely bloated settings file, which Xcode seems to even manipulate and change from time to time out of fun, even without the developer performing any change on purpose.

Did the requested changes, thanks for the review 👍

nerdsupremacist · 2017-04-16T21:01:39Z

@mammuth Is it ready? If not give me a heads up when it is. So far everything in the code looks fine

nerdsupremacist

I have some style suggestions. But I don't mind. Merge whenever you're ready ;)

nerdsupremacist · 2017-04-16T21:04:55Z

TUM Campus App/StudyRoomsTableViewController.swift

+extension StudyRoomsTableViewController: TumDataReceiver {
+
+    func receiveData(_ data: [DataElement]) { // Is called twice, once with [StudyRoomGroups] as argument, once for [StudyRooms]
+        for item in data {


Since we're already hunting down uncessesary loops:

roomGroups.append(contentsOf: data.flatMap { $0 as? StudyRoomGroup }) studyRooms.append(contentsOf: data.flatMap { $0 as? StudyRoom })

should do it ;)

nerdsupremacist · 2017-04-16T21:06:47Z

TUM Campus App/StudyRoomsTableViewController.swift

+        didSet {
+            title = currentGroup?.name
+            currentRooms = self.studyRooms
+                .filter() { (self.currentGroup?.roomNumbers.contains($0.roomNumber)) ?? false }


you don't need the () when the only argument is a trailing closure.

nerdsupremacist · 2017-04-16T21:10:56Z

TUM Campus App/StudyRoomsManager.swift

+                                if let data = response.result.value {
+                                    if let json = JSON(data).dictionary {
+                                        if let rooms = json["raeume"]?.array {
+                                            StudyRoomsManager.studyRooms.removeAll()


Well then the loops are unnecesary. The flatMap does all the work for you

mammuth · 2017-04-16T21:17:30Z

I have some style suggestions. But I don't mind. Merge whenever you're ready ;)

I give you all the power and responsibility 😆

Merge it, or add your style suggestions. Maybe just take a look that we get it into master before 20.4. so we can ship it with the next release.

Added StudyRoom basic functionality

ee3d0c8

mammuth added the not-ready label Dec 18, 2016

mammuth self-assigned this Dec 18, 2016

mammuth added 4 commits December 18, 2016 16:36

Link StudyRooms to their RoomFinder Maps onClick.

af7b595

I therefore extended The RoomFinder VC to handle StudyRooms as well.

Add PullToRefresh to StudyRooms

4a882e7

As a possible downside the fetched rooms are not cached, when leaving and reentering the study rooms view. However this probably makes sense and should have been this way anyway.

Add group picker to StudyRooms

991f9a3

Fix duplicated groups when using pull to refresh

00691b8

mammuth assigned nerdsupremacist and unassigned mammuth Dec 19, 2016

kordianbruck mentioned this pull request Dec 19, 2016

Plans feature #50

Closed

Trigger build

2a9963d

mammuth mentioned this pull request Mar 31, 2017

Study Rooms #24

Closed

mammuth added 2 commits April 2, 2017 08:08

Move occupied indicator color to the status

45d444e

Merge branch 'master' into feature/24-study-rooms

9a2ed87

# Conflicts: # TUM Campus App.xcodeproj/project.pbxproj # TUM Campus App/More.storyboard # TUM Campus App/TumDataItems.swift

mammuth added 3 commits April 2, 2017 08:52

Fix merge conflict in More.storyboard

69c27b2

Remove building, some design changes

28b0ee8

Pods update and maybe some parts of the master merge

30b61bb

mammuth requested a review from nerdsupremacist April 2, 2017 07:00

Probably fix the build

a7c78d1

Don’t know why, but the run configurations have not been properly set for the testing targets

Merge branch 'master' into feature/24-study-rooms

6a61171

nerdsupremacist suggested changes Apr 15, 2017

View reviewed changes

mammuth added 4 commits April 16, 2017 21:26

Delete fastlane/Deliverfile

6871dc5

since it is personal and doesn’t contain any meaningful settings (yet)

Map StudyRoom.init instead of using removeAll()

2f46b7b

Use correct casing in the style constants

74afdf3

Add layout constraints

68a5aab

nerdsupremacist approved these changes Apr 16, 2017

View reviewed changes

nerdsupremacist suggested changes Apr 16, 2017

View reviewed changes

Some more syntax finetuning

c6a9163

mammuth removed the not-ready label Apr 16, 2017

Remove unneeded loops in StudyRoomManager

22d659f

nerdsupremacist approved these changes Apr 16, 2017

View reviewed changes

Merge branch 'master' into feature/24-study-rooms

f499f9f

nerdsupremacist changed the title ~~[WIP] StudyRoom Feature~~ StudyRoom Feature Apr 16, 2017

Merge branch 'master' into feature/24-study-rooms

6649b76

nerdsupremacist merged commit db94c38 into TUM-Dev:master Apr 16, 2017

mammuth deleted the feature/24-study-rooms branch April 17, 2017 08:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StudyRoom Feature #49

StudyRoom Feature #49

mammuth commented Dec 18, 2016 •

edited

kordianbruck commented Dec 18, 2016 •

edited

nerdsupremacist commented Dec 19, 2016

nerdsupremacist commented Dec 19, 2016

mammuth commented Dec 19, 2016 via email

mammuth commented Mar 3, 2017

mammuth commented Mar 31, 2017

mammuth commented Apr 2, 2017 •

edited

mammuth commented Apr 2, 2017 •

edited

kordianbruck commented Apr 2, 2017

nerdsupremacist commented Apr 15, 2017 •

edited

nerdsupremacist Apr 15, 2017

mammuth Apr 16, 2017

nerdsupremacist Apr 16, 2017

nerdsupremacist Apr 15, 2017

nerdsupremacist Apr 15, 2017

mammuth Apr 16, 2017

nerdsupremacist Apr 16, 2017

nerdsupremacist Apr 16, 2017

mammuth commented Apr 16, 2017 •

edited

nerdsupremacist commented Apr 16, 2017

nerdsupremacist left a comment

nerdsupremacist Apr 16, 2017

nerdsupremacist Apr 16, 2017

nerdsupremacist Apr 16, 2017

mammuth commented Apr 16, 2017 •

edited

StudyRoom Feature #49

StudyRoom Feature #49

Conversation

mammuth commented Dec 18, 2016 • edited

Big ToDo's:

kordianbruck commented Dec 18, 2016 • edited

nerdsupremacist commented Dec 19, 2016

nerdsupremacist commented Dec 19, 2016

mammuth commented Dec 19, 2016 via email

mammuth commented Mar 3, 2017

mammuth commented Mar 31, 2017

mammuth commented Apr 2, 2017 • edited

mammuth commented Apr 2, 2017 • edited

kordianbruck commented Apr 2, 2017

nerdsupremacist commented Apr 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mammuth commented Apr 16, 2017 • edited

nerdsupremacist commented Apr 16, 2017

nerdsupremacist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mammuth commented Apr 16, 2017 • edited

mammuth commented Dec 18, 2016 •

edited

kordianbruck commented Dec 18, 2016 •

edited

mammuth commented Apr 2, 2017 •

edited

mammuth commented Apr 2, 2017 •

edited

nerdsupremacist commented Apr 15, 2017 •

edited

mammuth commented Apr 16, 2017 •

edited

mammuth commented Apr 16, 2017 •

edited