Skip to content

Add Set Favorite account functionality#91

Open
stupidly-logical wants to merge 8 commits into
Pritom14:masterfrom
stupidly-logical:favorite-account
Open

Add Set Favorite account functionality#91
stupidly-logical wants to merge 8 commits into
Pritom14:masterfrom
stupidly-logical:favorite-account

Conversation

@stupidly-logical

Copy link
Copy Markdown
Contributor

Changes for functionality requested in #67

@Pritom14

Pritom14 commented Oct 3, 2018

Copy link
Copy Markdown
Owner

If there are changes in the UI could you please add the screenshot in the screenshot folder

For Added Favorite account functionality
@stupidly-logical

Copy link
Copy Markdown
Contributor Author

Added.

@Pritom14

Pritom14 commented Oct 4, 2018

Copy link
Copy Markdown
Owner

The only problem with this PR is that when I set a favorite account by clicking on the star icon, it sets it to favorite (by filling the star icon). But when I go on adding a new account, it deselects it from favorite.
Could you please look into the issue?

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

Will check.

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

@Pritom14 Issue fixed.

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

@Pritom14 Checked?

@Pritom14

Pritom14 commented Oct 8, 2018

Copy link
Copy Markdown
Owner

@stupidly-logical I have mentioned the change to be made. Just remove the private keyword

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

@Pritom14 I can't find where you've mentioned. Kindly guide.

@Pritom14

Pritom14 commented Oct 8, 2018

Copy link
Copy Markdown
Owner

@stupidly-logical go to the Files Changed tab, and then go to the PasswordRecyclerViewAdapter class where you will find the review at line no. 202

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

That ImageView needed to be added because of PR Quality Issue.

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

@Pritom14

Pritom14 commented Oct 8, 2018

Copy link
Copy Markdown
Owner

@stupidly-logical , since @BindView fields cannot be private or static, so just remove the private keyword. So after changes the line would be ImageView iconFav;

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

But then it will not pass check in Codacy check like this. : https://app.codacy.com/app/Pritom14/Password-Storage/file/23570889520/issues/source?fileBranchId=9037891

@Pritom14

Pritom14 commented Oct 8, 2018

Copy link
Copy Markdown
Owner

Did you run this in Android-Studio, because when I pull the PR, it shows an error stating that @BindViews cannot be private or static

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

Yes but for passing the Code Review test I had to make it private.

If you are okay with the Code Review error, I'll remove the private keyword and push?

@Pritom14

Pritom14 commented Oct 8, 2018

Copy link
Copy Markdown
Owner

Okay, let it be. I will commit the change from my side

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

No problem. I am pushing the change.

@Pritom14

Pritom14 commented Oct 8, 2018

Copy link
Copy Markdown
Owner

@stupidly-logical , don't!!!. Let it be private. I will make the change from my side

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

Ok.

@Pritom14

Pritom14 commented Oct 8, 2018

Copy link
Copy Markdown
Owner

Will it be ok, if I give the review on the code tomorrow, because the device on which I am testing the app is having some problem.

@Pritom14

Pritom14 commented Oct 8, 2018

Copy link
Copy Markdown
Owner

Till then you could check out more projects or could find out more issues to work on

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

Completely fine with me. Take your time. 👍

@Pritom14

Pritom14 commented Oct 9, 2018

Copy link
Copy Markdown
Owner

@stupidly-logical have you tested this in an android device, because I am not getting the desired result. If there is some favorite account set, and you create a new account in the app, the favorite icon unset by itself. Also I am unable to unset the favorite account manually

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

Yes I was getting the desired result and I also fixed the issue that you pointed out that it was setting it back to not favorite when you add a new account. Give me some time. I'll check once again. Please make sure that you pulled my branch properly too.

@Pritom14

Pritom14 commented Oct 9, 2018 via email

Copy link
Copy Markdown
Owner

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

Ok. I'll check too.

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

@Pritom14 Meanwhile I've found some issues when I analyzed the code. I'll create an issue and make PR.

@stupidly-logical

Copy link
Copy Markdown
Contributor Author

@Pritom14 I've removed private keyword and tested the functionality. It is working as desired.

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.

2 participants