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

Add tag filtering options #12

Merged
merged 11 commits into from
Oct 31, 2017
Merged

Add tag filtering options #12

merged 11 commits into from
Oct 31, 2017

Conversation

ItsT-Mo
Copy link
Contributor

@ItsT-Mo ItsT-Mo commented Oct 10, 2017 •
edited
Loading

Allows issue filtering based on tags.

Fixes issue #1

Empty2k12
Empty2k12 suggested changes Oct 11, 2017
View reviewed changes
Copy link
Contributor

@Empty2k12 Empty2k12 left a comment

Choose a reason for hiding this comment

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

Code Style review

app/src/main/java/com/naman14/hacktoberfest/fragment/ExploreFragment.java Outdated
new MaterialDialog.Builder(getActivity())
.title("Select Tags")
.items(Utils.getTagsArray())
.itemsCallbackMultiChoice(tagsPreferenceIndex
Copy link
Contributor

Choose a reason for hiding this comment

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

weird formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 58883bc

app/src/main/java/com/naman14/hacktoberfest/utils/Utils.java Outdated
@@ -10,12 +10,14 @@
import android.preference.PreferenceManager;
import android.support.customtabs.CustomTabsIntent;
import android.text.TextUtils;
import android.util.Log;
Copy link
Contributor

Choose a reason for hiding this comment

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

Development Import

Copy link
Contributor Author

@ItsT-Mo ItsT-Mo Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I missed that one, thanks! :)

Fixed in 7cb619a

app/src/main/res/layout/layout_filter_reveal.xml
<TextView
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:text="Select Tags"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use @Strings here to be future proof for #9

app/src/main/res/layout/layout_filter_reveal.xml
<TextView
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:text="Select Language"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use @Strings here to be future proof for #9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When developing I actively decided against that. I saw there was a PR open that was adressing that, but I decided to stick with the current way Strings are embedded into the app (namely hardcoded).

I did this for 2 reasons:
a) In case he does not want to use string ressources (unlikely)
b) So when he merges this branch with the master branch it will be consistent.

I would rather use string ressources aswell. If #9 gets integrated, I will update these values with string ressource values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a fair plan, you can always rebase against master later!

@Empty2k12
Copy link
Contributor

Empty2k12 commented Oct 11, 2017 •
edited
Loading

I built your branch but on first opening the app, it crashed:

E  java.lang.NullPointerException: Attempt to get length of null array
E      at com.naman14.hacktoberfest.utils.Utils.getTagsPreferenceString(Utils.java:164)
E      at com.naman14.hacktoberfest.fragment.ExploreFragment.setupFilter(ExploreFragment.java:118)
E      at com.naman14.hacktoberfest.fragment.ExploreFragment.onCreateView(ExploreFragment.java:87)
E      at android.support.v4.app.Fragment.performCreateView(Fragment.java:2343)
E      at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1419)
E      at android.support.v4.app.FragmentManagerImpl.moveFragmentToExpectedState(FragmentManager.java:1740)
E      at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1809)
E      at android.support.v4.app.BackStackRecord.executeOps(BackStackRecord.java:799)
E      at android.support.v4.app.FragmentManagerImpl.executeOps(FragmentManager.java:2580)
E      at android.support.v4.app.FragmentManagerImpl.executeOpsTogether(FragmentManager.java:2367)
E      at android.support.v4.app.FragmentManagerImpl.removeRedundantOperationsAndExecute(FragmentManager.java:2322)
E      at android.support.v4.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:2229)
E      at android.support.v4.app.FragmentManagerImpl$1.run(FragmentManager.java:700)
E      at android.os.Handler.handleCallback(Handler.java:751)
E      at android.os.Handler.dispatchMessage(Handler.java:95)
E      at android.os.Looper.loop(Looper.java:154)
E      at android.app.ActivityThread.main(ActivityThread.java:6334)
E      at java.lang.reflect.Method.invoke(Native Method)
E      at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
E      at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

@ItsT-Mo
Copy link
Contributor Author

ItsT-Mo commented Oct 11, 2017 •
edited
Loading

Found the issue and fixed it.

Problem was, I was returning null when no tags preference was set. Returning an emtpy String array is better in this case, since no saved value means no preferences. Thanks for the report! :)

Fixed in 7509f85

@Empty2k12
Copy link
Contributor

Empty2k12 commented Oct 11, 2017 •
edited
Loading

With your latest fixes this is working great. Thanks for the contribution!

One simple improvement could be moving down the whole filter selectors in the blue circle to account for the bigger spaces needed. So simple move down the layout to center it in the blue circle!

@ItsT-Mo
Copy link
Contributor Author

ItsT-Mo commented Oct 11, 2017

Would you mind posting a screenshot of how it looks on your phone? Because on mine, the blue circle encompasses the whole filter selectors. :)

@Empty2k12
Copy link
Contributor

screenshot_20171011-150548

This is how it looks. In my opinion it could be moved down a bit further?

@naman14 naman14 merged commit 58883bc into naman14:master Oct 31, 2017
@naman14 naman14 mentioned this pull request Oct 3, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Empty2k12 Empty2k12 Empty2k12 requested changes

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

Successfully merging this pull request may close these issues.
3 participants
@ItsT-Mo @Empty2k12 @naman14