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.

Sign up for GitHub

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

Jump to bottom

Added Favourites section #607

Merged
merged 7 commits into from
Mar 10, 2019
Merged

Added Favourites section #607

merged 7 commits into from
Mar 10, 2019

Conversation

jddeep
Copy link
Contributor

@jddeep jddeep commented Mar 7, 2019 •
edited
Loading

Description.

screenshot_20190307-210426

screenshot_20190307-210430

Fixes #563

Type of change

Just put an x in the [] which are valid.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • ./gradlew assembleDebug assembleRelease
  • ./gradlew checkstyle

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@codeclimate
Copy link

codeclimate bot commented Mar 7, 2019

Code Climate has analyzed commit 8993b1f and detected 20 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 2
Duplication 18

View more on Code Climate.

@Swati4star Swati4star requested a review from vyankatesh24 March 7, 2019 16:01
dr0pdb
dr0pdb suggested changes Mar 7, 2019
View reviewed changes
Copy link

@dr0pdb dr0pdb left a comment

Choose a reason for hiding this comment

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

Too many formatting mistakes. Always check for these mistakes in the files changed section after opening a PR.

@jddeep
Copy link
Contributor Author

jddeep commented Mar 8, 2019

Okay I will look into all changes. @Swati4star please tell what do you think about the replies which I gave to some above comments. Thanks

@jddeep jddeep force-pushed the favouritesection branch from 8993b1f to 7307a2a Compare March 9, 2019 09:40
@jddeep
Copy link
Contributor Author

jddeep commented Mar 9, 2019

@Swati4star Please review now.

Swati4star
Swati4star requested changes Mar 9, 2019
View reviewed changes
app/src/main/java/swati4star/createpdf/fragment/FavouritesFragment.java Outdated
implements SharedPreferences.OnSharedPreferenceChangeListener {

private Activity mActivity;
private SharedPreferences mSharedPreferences;
Copy link
Owner

Choose a reason for hiding this comment

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

we already have this instance of SharedPreferences. Use this. Don't create new variable for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah will remove it.

Copy link
Owner

Choose a reason for hiding this comment

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

You werent supposed to remove mSharedPreferences

app/src/main/java/swati4star/createpdf/fragment/FavouritesFragment.java Outdated
* @param view
* @param id
*/
private void viewVisibility(SharedPreferences sharedPreferences, View view, String id) {
Copy link
Owner

Choose a reason for hiding this comment

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

no need to receive SharedPreferences parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we need it to get the boolean value for a given id

Copy link
Owner

Choose a reason for hiding this comment

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

You will directly be able to use mSharedPreferences variable. There wont be any need to get this as a parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I got it now. I had misunderstood you.

app/src/main/res/values/strings.xml Outdated
@@ -85,6 +85,7 @@
<string name="successToast">Images successfully added.</string>

<!-- Navigation drawer strings -->
<string name="img_to_pdf">Images to PDF</string>
Copy link
Owner

Choose a reason for hiding this comment

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

why is this change required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is used as a key

Copy link
Owner

Choose a reason for hiding this comment

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

We dont need translations of keys, so we add keys in a constants file.
https://github.com/Swati4star/Images-to-PDF/blob/master/app/src/main/java/swati4star/createpdf/util/Constants.java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay will change it then.

@jddeep jddeep mentioned this pull request Mar 10, 2019
Open
Swati4star
Swati4star reviewed Mar 10, 2019
View reviewed changes
app/src/main/java/swati4star/createpdf/fragment/FavouritesFragment.java Outdated
Bundle savedInstanceState) {
View rootview = inflater.inflate(R.layout.favourites_fragment, container, false);
ButterKnife.bind(this, rootview);
SharedPreferences sharedPreferences = PreferenceManager
Copy link
Owner

Choose a reason for hiding this comment

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

instead of creating sharedPreferences, I asked you to use previous mSharedPreferences variable. You have deleted it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood the other way around.

@jddeep
Copy link
Contributor Author

jddeep commented Mar 10, 2019

@Swati4star Do tell if there are any other changes. I will make a final commit tonight. Thanks

@Swati4star
Copy link
Owner

I already mentioned all the required changes.

@jddeep
Copy link
Contributor Author

jddeep commented Mar 10, 2019

Yeah other than those.

jd003walker and others added 2 commits March 10, 2019 12:58
@jddeep
Copy link
Contributor Author

jddeep commented Mar 10, 2019

@Swati4star All changes have been seen into. Thanks!

dr0pdb
dr0pdb approved these changes Mar 10, 2019
View reviewed changes
Copy link

@dr0pdb dr0pdb left a comment

Choose a reason for hiding this comment

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

Looks good

@Swati4star
Copy link
Owner

Awesome @jddeep 👍
Took a lot of iterations! Good job on accommodating all the changes.

@Swati4star Swati4star merged commit 1f8c465 into Swati4star:master Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Swati4star Swati4star Swati4star approved these changes

@dr0pdb dr0pdb dr0pdb approved these changes

@vyankatesh24 vyankatesh24 Awaiting requested review from vyankatesh24

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Favourites section on Home Page at the top
3 participants
@jddeep @Swati4star @dr0pdb