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

Add Set Favorite account functionality #91

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

stupidly-logical
Copy link
Contributor

Changes for functionality requested in #67

@Pritom14
Copy link
Owner

Pritom14 commented Oct 3, 2018

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
Contributor Author

Added.

@Pritom14
Copy link
Owner

Pritom14 commented Oct 4, 2018

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
Contributor Author

Will check.

@stupidly-logical
Copy link
Contributor Author

@Pritom14 Issue fixed.

@stupidly-logical
Copy link
Contributor Author

@Pritom14 Checked?

@Pritom14
Copy link
Owner

Pritom14 commented Oct 8, 2018

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

@stupidly-logical
Copy link
Contributor Author

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

@Pritom14
Copy link
Owner

Pritom14 commented Oct 8, 2018

@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
Contributor Author

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

@Pritom14
Copy link
Owner

Pritom14 commented Oct 8, 2018

@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
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
Copy link
Owner

Pritom14 commented Oct 8, 2018

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
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
Copy link
Owner

Pritom14 commented Oct 8, 2018

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

@stupidly-logical
Copy link
Contributor Author

No problem. I am pushing the change.

@Pritom14
Copy link
Owner

Pritom14 commented Oct 8, 2018

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

@stupidly-logical
Copy link
Contributor Author

Ok.

@Pritom14
Copy link
Owner

Pritom14 commented Oct 8, 2018

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
Copy link
Owner

Pritom14 commented Oct 8, 2018

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

@stupidly-logical
Copy link
Contributor Author

Completely fine with me. Take your time. 👍

@Pritom14
Copy link
Owner

Pritom14 commented Oct 9, 2018

@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
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
Copy link
Owner

Pritom14 commented Oct 9, 2018 via email

@stupidly-logical
Copy link
Contributor Author

Ok. I'll check too.

@stupidly-logical
Copy link
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
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