-
Notifications
You must be signed in to change notification settings - Fork 9
Feature/keyboard commands #30
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
base: Development
Are you sure you want to change the base?
Feature/keyboard commands #30
Conversation
This reverts commit 289ce8d.
| options: [.allowUserInteraction, .curveEaseInOut], | ||
| animations: { | ||
| self.hostingViewController?.view.layoutIfNeeded() | ||
| self.draggerView.backgroundColor = #colorLiteral(red: 0.0862745098, green: 0.0862745098, blue: 0.09411764706, alpha: 1) |
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.
warum übergibst du die Farbe als Parameter?
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.
changeHeight wird von expand() und collapse() aufgerufen, und beim Collapsen möchtest du den Dragger transparent machen, und beim Expandieren schwarz.
Die Aufgabe der Methode ist es, die Höhe zu ändern, nicht die Farbe zu bestimmen.
Farbbestimmung könnte man in eine eigene Methode auslagern.
| collectionView.scrollToItem(at: newIndexPath, at: .centeredHorizontally, animated: true) | ||
| } | ||
|
|
||
| @objc func upvoteCurrentPost() { |
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.
warum die ganzen actions auf dem DetailCollectionViewController und nicht auf dem DetailViewController?
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.
Weil du CollectionView verwendest, hast du immer mehrere aktive DetailViewController.
Daher werden die Shortcuts auf dem DetailCollectionViewController alleine abgefangen und dann an den jeweils aktiven DetailViewController weitergeleitet.
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.
Ansonsten würden alle aktiven DetailViewController den Shortcut bekommen und alle dann Blussi vergeben oder was auch immer.
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.
Ah ja stimmt, überlege grad wie man das noch optimieren könnte...
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.
Ich wollte übrigens zuerst das Observer Pattern verwenden, aber das scheiterte aus dem gleichen Grund.
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.
UIPageViewController löst wie gesagt diese ganzen Probleme. Da wird viewWillDisappear und der ganze andere Schnutzbutz gleich gefeuert, wenn du auf die nächste Seite wischst.
UICollectionView ist halt nicht für solche Fullscreensachen gedacht.
Aber trotzdem damit wirklich cool umgesetzt.
| } | ||
|
|
||
| func upvotePost() { | ||
| self.viewModel.vote(.up) |
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.
self bitte nur wenn du es brauchst
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.
Ok, mein Codestil ist: Immer this/self verwenden, wenn Klassenvariablen verwendet werden, da man so leichter erkennen kann, welche Variablen im Methodenscope sind, und welche nicht.
Hat mich teilweise beim Lesen des Codes oft suchen lassen, wo was steht.
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.
Dafür hast du code highlighting. Für mich ist das boilerplate... :D
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.
Ja ok, ändere ich.
Ich komme aus der C#-Welt, da ist das branchenübliche Best Practice.
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.
Erledigt.
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.
| self.didFinishDownvote() | ||
| } | ||
|
|
||
| func favouritePost() { |
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.
American English bitte :D
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.
Ok, ich wohne halt in Europa, daher nehm ich immer British English
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.
aber wenn schon favorite drunter :D
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.
Erledigt.
[TASK] Change favourite to favorite.

This PR adds support for keyboard shortcuts and closes issue #26.