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

Sort the transfer accounts by favorite before sorting them alphabetically #622

Merged
merged 6 commits into from
Dec 29, 2016

Conversation

davidlandry93
Copy link
Contributor

This is a series of commits that puts the favorite accounts on top of the other accounts transfer accounts list. It makes it easier to add transactions if you use some transfer accounts more frequently.

I was annoyed at the app because I only really use two transfer accounts and I had to scroll through all my accouts to get to my credit card.

To avoid any confusion, I made it so that the favorite "star" would show beside the name of the transfer accounts that are favorite. That way the user knows why they appear first.

Thanks for the consideration!

Copy link
Collaborator

@rivaldi8 rivaldi8 left a comment

Choose a reason for hiding this comment

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

Adapt the pull request according to the next comment. In doing so, avoid these small issues I've added inline in the code.

public void bindView(View view, Context context, Cursor cursor) {
super.bindView(view, context, cursor);

Integer is_favorite = cursor.getInt(cursor.getColumnIndex(DatabaseSchema.AccountEntry.COLUMN_FAVORITE));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, try to follow Java coding conventions. Variables should use camel case. For example, isFavorite instead of is_favorite.

@@ -0,0 +1,24 @@
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, try to fix the editor warnings if possible. For example, in this line it says the <LinearLayout> and its contents can be replaced by a single <TextView> with a drawableEnd attribute.

@rivaldi8
Copy link
Collaborator

rivaldi8 commented Dec 6, 2016

Thanks for the pull request! I think it's a good idea.

I've checked all the uses of QualifiedAccountNameCursorAdapter and I think all of them would be improved if the favorite accounts were listed first and with the star. So instead of implementing it as a separate FavoritableQualifiedAccountNameCursorAdapter class, just add the feature to QualifiedAccountNameCursorAdapter.

There's one thing I don't like, though. I agree that the favorite stars help to avoid confusion about why those entries appear first when showing the list. However, when a favorite account is selected, the star is still shown. I think it stands out too much and adds clutter. Also it looked a bit confusing when I first saw it. Could you try to hide it when the spinner list closed. It may be possible to do so from AdapterView.OnItemSelectedListener.onItemSelected. If it's not possible, I'd rather not show it for now.

Please, let me know if you need any help with this.

@davidlandry93
Copy link
Contributor Author

I think you are right, I'll look into it this weekend probably. Thanks!

@rivaldi8 rivaldi8 merged commit af0c657 into codinguser:develop Dec 29, 2016
@rivaldi8
Copy link
Collaborator

Thanks! Merged.

@codinguser codinguser added this to the 2.1.5 milestone Mar 27, 2017
This pull request was closed.
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.

3 participants