Add pull to refresh. Also add Butterknife. #18
Conversation
There was a problem hiding this comment.
Thanks! This will be great.
Let's keep the Refresh button in the menu for accessibility - but you can hide it in overflow instead of showing the icon in the appbarlayout. Ref: https://developer.android.com/training/swipe/add-swipe-interface.html#AddRefreshAction.
| } else if (!TextUtils.isEmpty(mZip)) { | ||
| AppSingleton.getInstance(getApplicationContext()).getJsonController() | ||
| .getIssuesForLocation(mZip); | ||
| } |
There was a problem hiding this comment.
Could you add a TODO for a "location error" else block, if we've got neither zip nor lat/long? It shouldn't happen but...
There was a problem hiding this comment.
Do you want me to just add it? What should we do in that case? Toast the user and log an error to the backend?
There was a problem hiding this comment.
I'm not sure, ultimately I guess we'd want to redirect them to the Location screen to try to reset it, but a snackbar error sounds good for now.
(I prefer Snackbar to Toast as Snackbar meets accessibility standards.)
There was a problem hiding this comment.
Ok cool, sounds good
| /** | ||
| * The activity which handles zip code lookup and showing the issues list. | ||
| * | ||
| * TODO: Add loading spinners when making Volley requests. | ||
| * |
There was a problem hiding this comment.
Maybe a TODO to use Butterknife across the app for consistency?
There was a problem hiding this comment.
Yep! I can do that in the next PR.
|
@dektar Addressed your comments. I also fixed a crash I encountered multiple times, when returning to the LocationActivity from a permissions request. When |
| startActivity(intent); | ||
| finish(); | ||
| } | ||
| Intent intent = new Intent(this, MainActivity.class); |
There was a problem hiding this comment.
So I think that if you return always with an intent, we end up with a big stack of activities (Location / Main / Location / Main). Even if you finish(), you end up with a stack of Main activities.
Curious why you changed this from the if (mFromMain) { onBackPressed() }?
There was a problem hiding this comment.
Oh, I see your explanation above. Can we move this into a different PR to keep it separate from all this functionality? I'd like to experiment and see if there's a way to make it work without multiple layers of activities.
One option may be that we "startActivityForResult" from MainActivity when the Location menu option is clicked, and then we can "onActivityResult" instead of "onBackPressed". I'd need to look into it more, though.
Or, I do want to consider refactoring to using Fragments at some point... maybe starting with this class would make sense?
There was a problem hiding this comment.
Cool, I'll pull that out.
| tools:context="org.a5calls.android.a5calls.controller.MainActivity"> | ||
|
|
||
| <android.support.v4.widget.SwipeRefreshLayout | ||
| android:id="@+id/swipeContainer" |
There was a problem hiding this comment.
I've been doing XML IDs like swipe_container instead of swipeContainer. Does this way work better with butterknife? Or can we stick with the other convention?
There was a problem hiding this comment.
Oh we can stick to the other convention. Doesn't matter to me.
Gregliest
force-pushed
the
feature/ptr
branch
from
d151c65
to
f9ddee2
Compare
|
@dektar Done. I'm just going to make an issue for the crash, since the brute force fix was trivial. |
@dektar
Adds pull to refresh to the MainActivity's RecyclerView. The spinner will show A) when the user enters the activity (in onResume) and B) when the user pulls to refresh. I removed the refresh button in the menu, let me know if you'd like me to add it back.
I also added Butterknife for convenience, so that we don't need to do a typecast +
findbyIdevery time we want a view. I did some minor refactoring, splitting the api listener logic inonCreateViewinto a helper.