Feature/filter by animal type #90

amirfl · 2018-04-13T17:24:28Z

Added filter by animal functionality.
Feature #65

EDIT:
I found a little bug introduced with this pull request, where endless scroll doesn't work properly with the filtered results.
trying to fix it Fixed :)
Now also fix #78 - Empty state placeholder

amirfl · 2018-04-13T19:12:45Z

PetAdoption-iOS/PetAdoption-iOS/AppDelegate.swift

@@ -57,10 +57,11 @@ class AppDelegate: UIResponder, UIApplicationDelegate {

 		navBar.barStyle = .black

-		UIButton.appearance().tintColor = UIColor.themeTintColor()


Had to comment this line out because it causes tableviewCell checkmark accessory views and button labels to be white on white (and therefore invisible).

I tried to solve this by setting the tint color of the table view and its cells locally, but that didn't work, so for now I just commented this line out.
Commenting this out, as a temporary solution, didn't seem to have any bad affect on the existing UI.

I would say just go ahead and remove the line instead. I'm not sure why we had UIButton appearances set to white. UIBarButtonItem appearances is fine to be white, since the navigation bar is not white.

Removed the UIButton tintColor appearance setting 4b6fb28

WERUreo

Overall this looks good. There are some nitpicky fixes, to be sure. I try not to harp too much on code style, which is why I only mentioned a couple of them. Also, if you can in the future, try not to add other issues into the work you're currently doing, especially if someone else is already working on that issue (talking about the empty state view). In this case, it's fine because it's a minor issue, but I don't want us to trample all over each other's work.

Other than that, thanks for working on this!

WERUreo · 2018-04-17T02:09:30Z

History.md

@@ -0,0 +1,26 @@
+


We probably don't necessarily need a History.md file, since we can view the history right in Github, but it does remind me that I should probably do release notes when we're ready to submit v1.1.

Removed History.md. I think I added it somehow by mistake

WERUreo · 2018-04-17T02:11:22Z

PetAdoption-iOS/PetAdoption-iOS/Controllers/filterViewControlelr.swift

@@ -0,0 +1,135 @@
+//
+//  filterViewControlelr.swift


This is probably a little nitpicky, but the file name is, for one, misspelled (Controlelr instead of Controller), and two, convention is to capitalize the names of the files.

Good eyes :)
fixed with 31da2d4

WERUreo · 2018-04-17T02:23:28Z

PetAdoption-iOS/PetAdoption-iOS/Controllers/PetListingViewController.swift

@@ -175,6 +182,21 @@ class PetListingViewController: UIViewController
        self.present(alertController, animated: true, completion: nil)
    }

+    func presentFilterViewController()
+    {
+        let filterVC = storyboard?.instantiateViewController(withIdentifier: "filterVC") as? FilterViewController


Unwrapping the optional right up here (either if let or with a guard) would prevent needing to force unwrap filterVC down on line 193.

WERUreo · 2018-04-17T02:28:30Z

PetAdoption-iOS/PetAdoption-iOS.xcodeproj/project.pbxproj

@@ -466,6 +476,7 @@
 		D0950AF71C618D86002E198C /* ViewControllers */ = {
 			isa = PBXGroup;
 			children = (
+				6F2A704A207EB56D00BF5AA0 /* filter */,


Again, being nitpicky, but all the other groups are capitalized.

Addressed in 8b4feb5

WERUreo · 2018-04-17T02:31:02Z

PetAdoption-iOS/PetAdoption-iOS/Controllers/PetListingViewController.swift

    }

    ////////////////////////////////////////////////////////////

    func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell
    {
        let cell = collectionView.dequeueReusableCell(withReuseIdentifier: PetCell.reuseIdentifier, for: indexPath) as! PetCell
-        let pet = petData[indexPath.row]
+        let pet = filteredPetData[indexPath.row]//petData[indexPath.row]


You can just remove the commented out code at the end of this line. Unless you left it there for a reason.

Removed with d39f3a1

WERUreo · 2018-04-17T02:32:27Z

PetAdoption-iOS/PetAdoption-iOS/Controllers/filterViewControlelr.swift

+class FilterViewController: UITableViewController {
+    var delegate: FilterSelectorDelegate?
+    enum RowIndex:Int {
+        case All = 0, Dogs, Cats, Birds, SmallFurry, Horses, BarnYard, Reptiles, Rabbits


The convention for enums in Swift 4 is that cases should begin with lowercase letters

Fixed with bd8b2ef

WERUreo · 2018-04-17T02:38:34Z

PetAdoption-iOS/PetAdoption-iOS/Controllers/PetListingViewController.swift

@@ -237,15 +261,16 @@ extension PetListingViewController : UICollectionViewDelegate, UICollectionViewD
 {
    func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int
    {
-        return self.petData.count
+        self.noResultsLabel.isHidden = !(self.filteredPetData.count == 0)


So with endless scroll, there is a collection view footer with a UIActivityIndicatorView in it. The purpose of that is to give the users an indication that there is something loading. If the user reaches the very bottom of the collection (2000 results), the activity indicator stops animating.

I noticed a bug whereby if there are no results found, that activity indicator is still animating at the very top. That's that footer with the activity indicator. We will probably need to add something somewhere to stop that animation if there are no results. I'm not sure where exactly to add this, but check out the viewForSupplementaryElementOfKind at indexPath function to see where we are currently starting and stopping that animation.

On another note, I am currently working on the empty view myself, but what I'll do is leave what you have here and then just add my changes afterwards.

Sorry for taking over the empty state issue. I saw it labeled as 'help needed' and assumed it means its ok to pick it up. I'm very new to the whole collaboration way of doing things so thanks for bearing with me :)

About the activity indicator, I think there is a related problem where if not enough animals of a selected type are loaded to fill the screen, a request to download more animals should be triggered, but isn't. This is because pets are loaded into petData, but displayed out of filteredPetData. Not sure what the best way to solve this would be.

Maybe a solution could be to call loadPets() many times while MAX_SEARCH_RESULTS hasn't bean reached ~~AND~~ OR filteredPetData < minFilteredResultAttempts ?
Any thoughts?

No worries about the empty state thing. Typically, if you see someone is assigned to an issue, usually that means they're currently looking into it. At the very least, reach out to that person to find out if they're actively working on an issue. Sometimes someone will assign themselves to an issue and then not have any time to actually work on it (happens to me all the time lol).

Let me think about the activity indicator problem a little bit. For now, I'm ok with leaving this minor bug in there unless we can come up with a solution before we end up pushing the next version.

Crap, you know what I just thought about? I wonder if it would be better for us to actually modify the API call to only return us a list of pets given the animal type. Going back to the API docs, we can optionally pass a string indicating what type of animals we want returned. The only issue with this I can see is that we may not be able to filter on multiple animal types. Well that and a lot of the work you've already put into this will need to be re-done.

One reason we might want to consider this approach is that because we are rate limited to 2000 results max, we are very likely leaving out a lot of results by default. We could potentially get more results, especially for the less common animals. This will still leave us with the activity indicator problem because, like you mention, the check for stopping that animation is based solely on the max results (2000).

Sorry for only now thinking about this. This is probably one of the reasons I had kept this issue out until v1.2. That and we probably will need to have a discussion about other filtering options we will want to eventually add in.

WERUreo · 2018-04-17T02:45:41Z

PetAdoption-iOS/PetAdoption-iOS/Controllers/filterViewControlelr.swift

+        defaults.synchronize()
+    }
+
+    func setAnimalTypeCheckmarks() {


Another bug I've noticed is that you can get into a situation where nothing is checked at all. If I select an animal type and then deselect it, now nothing is checked. It would be nice if in the case that nothing is checked, the "all" option becomes checked. Is that possible?

Fixed with 50a3b0d

WERUreo · 2018-04-17T02:48:01Z

PetAdoption-iOS/PetAdoption-iOS/AppDelegate.swift

@@ -57,10 +57,11 @@ class AppDelegate: UIResponder, UIApplicationDelegate {

 		navBar.barStyle = .black

-		UIButton.appearance().tintColor = UIColor.themeTintColor()


I would say just go ahead and remove the line instead. I'm not sure why we had UIButton appearances set to white. UIBarButtonItem appearances is fine to be white, since the navigation bar is not white.

…ller.swift

…s loading

…ed array > 0 or max limit reached

WERUreo · 2018-04-22T18:46:15Z

PetAdoption-iOS/PetAdoption-iOS/Controllers/PetListingViewController.swift

@@ -175,6 +184,21 @@ class PetListingViewController: UIViewController
        self.present(alertController, animated: true, completion: nil)
    }

+    func presentFilterViewController()
+    {
+        guard let filterVC = storyboard?.instantiateViewController(withIdentifier: "filterVC") as? FilterViewController else {fatalError("Error: could not instantiate filterVC")}


I don't know that I would necessarily want us to fatalError here if the view controller can't be instantiated, since that will just crash the app. I also don't know that I'd want this to silently return either... I guess for now this is fine. At the very least, if this guard does fail, Crashlytics should report the crash and we can investigate further.

WERUreo · 2018-04-22T18:58:31Z

PetAdoption-iOS/PetAdoption-iOS/Controllers/PetListingViewController.swift

@@ -237,15 +261,16 @@ extension PetListingViewController : UICollectionViewDelegate, UICollectionViewD
 {
    func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int
    {
-        return self.petData.count
+        self.noResultsLabel.isHidden = !(self.filteredPetData.count == 0)


Crap, you know what I just thought about? I wonder if it would be better for us to actually modify the API call to only return us a list of pets given the animal type. Going back to the API docs, we can optionally pass a string indicating what type of animals we want returned. The only issue with this I can see is that we may not be able to filter on multiple animal types. Well that and a lot of the work you've already put into this will need to be re-done.

One reason we might want to consider this approach is that because we are rate limited to 2000 results max, we are very likely leaving out a lot of results by default. We could potentially get more results, especially for the less common animals. This will still leave us with the activity indicator problem because, like you mention, the check for stopping that animation is based solely on the max results (2000).

Sorry for only now thinking about this. This is probably one of the reasons I had kept this issue out until v1.2. That and we probably will need to have a discussion about other filtering options we will want to eventually add in.

amirfl added 11 commits April 11, 2018 13:41

add filter Icon

636b506

add filter bar button item

0e9b375

add popover with filter view controller

775ddf0

remove unnecesary navigation controller

cc0e8c1

add filter view controller class

ef09dd4

remove appearance for UIButton

08152f7

add filterVC

0c852ca

handle animal type selection

4096a22

add save and load selectedAnimalTypes to userdefaults

cb3f6fe

add FilterSelectorDelegate

ce029b4

add filtering functionality

317abea

amirfl commented Apr 13, 2018

View reviewed changes

amirfl added 6 commits April 13, 2018 15:21

fix segue to detail not using filtered results

6123e6e

remove print statement

7c7a70a

fix endless scrolling not working when watching filtered results

759d558

add no results placeholder

cb3358e

add hide/show functionality to 'no results found' placeholder label

b621d78

add 'Rabbits' to filter options

77066d7

WERUreo requested changes Apr 17, 2018

View reviewed changes

amirfl added 11 commits April 17, 2018 21:14

remove appearance tintColor setting for UIButton

4b6fb28

remove History.md

f15d898

fix and change name of filterViewControlelr.swift to FilterViewContro…

31da2d4

…ller.swift

refactor filterVC optional unwrapping with guard let statement

1ef2470

refactor filter to Filter

8b4feb5

remove comment

d39f3a1

refactor enum

bd8b2ef

Fix: user can't deselect all animal types

50a3b0d

Extract load more items into new function

363b04b

add protection: do not allow fetching data in loadPets if currently i…

de972fc

…s loading

fix filtered pets not loading by adding more fetch calls until filter…

94889e0

…ed array > 0 or max limit reached

WERUreo reviewed Apr 22, 2018

View reviewed changes

WERUreo requested changes Apr 22, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/filter by animal type #90

Feature/filter by animal type #90

amirfl commented Apr 13, 2018 •

edited

Loading

amirfl Apr 13, 2018

WERUreo Apr 17, 2018

amirfl Apr 18, 2018

WERUreo left a comment

WERUreo Apr 17, 2018

amirfl Apr 18, 2018

WERUreo Apr 17, 2018

amirfl Apr 18, 2018

WERUreo Apr 17, 2018

WERUreo Apr 17, 2018

amirfl Apr 18, 2018

WERUreo Apr 17, 2018

amirfl Apr 18, 2018

WERUreo Apr 17, 2018

amirfl Apr 18, 2018

WERUreo Apr 17, 2018

WERUreo Apr 17, 2018

amirfl Apr 18, 2018 •

edited

Loading

WERUreo Apr 22, 2018

WERUreo Apr 22, 2018

WERUreo Apr 17, 2018

amirfl Apr 18, 2018

WERUreo Apr 17, 2018

WERUreo Apr 22, 2018

WERUreo Apr 22, 2018

		@@ -57,10 +57,11 @@ class AppDelegate: UIResponder, UIApplicationDelegate {

		navBar.barStyle = .black

		UIButton.appearance().tintColor = UIColor.themeTintColor()

Feature/filter by animal type #90

Are you sure you want to change the base?

Feature/filter by animal type #90

Conversation

amirfl commented Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WERUreo 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfl Apr 18, 2018 • edited Loading

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

amirfl commented Apr 13, 2018 •

edited

Loading

amirfl Apr 18, 2018 •

edited

Loading