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

feat: sort attendee list on the basis of ticket type (#781) #782

Merged
merged 2 commits into from
Mar 29, 2018

Conversation

AkshayCHD
Copy link
Contributor

@AkshayCHD AkshayCHD commented Mar 16, 2018

Fixes #781 #275

Changes:Add sorting feature to sort attendee list on the basis of ticket type

Screenshots for the change:
screenshot_1522240633
screenshot_1522345453

@open-event-bot
Copy link

Hi @AkshayCHD!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@open-event-bot
Copy link

Hi @AkshayCHD!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@open-event-bot
Copy link

Hi @AkshayCHD!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@AkshayCHD
Copy link
Contributor Author

@iamareebjamal please review

@codecov
Copy link

codecov bot commented Mar 16, 2018

Codecov Report

Merging #782 into development will decrease coverage by 0.28%.
The diff coverage is 0%.

Impacted file tree graph

@@                Coverage Diff                @@
##             development     #782      +/-   ##
=================================================
- Coverage          32.52%   32.24%   -0.29%     
  Complexity           449      449              
=================================================
  Files                122      122              
  Lines               3557     3588      +31     
  Branches             127      129       +2     
=================================================
  Hits                1157     1157              
- Misses              2354     2385      +31     
  Partials              46       46
Impacted Files Coverage Δ Complexity Δ
...vent/app/core/attendee/list/AttendeesFragment.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...nt/app/core/attendee/list/StickyHeaderAdapter.java 0% <0%> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e91d338...4f8ff6e. Read the comment docs.

Copy link
Member

@iamareebjamal iamareebjamal left a comment

Choose a reason for hiding this comment

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

Use fastadapter for sorting

@@ -19,5 +19,8 @@
android:title="@string/filter_by_sync" />
</menu>
</item>
<item android:id="@+id/sort"
android:title="@string/sort" />
Copy link
Member

Choose a reason for hiding this comment

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

Don't use explicit dialog box, see above how the filter menu is created

@AkshayCHD
Copy link
Contributor Author

@iamareebjamal sorting in fastadapter requires min api level 24

@iamareebjamal
Copy link
Member

Where is it written so, it is a library and hence shouldn't have any min API constraint

@AkshayCHD
Copy link
Contributor Author

@iamareebjamal the sort function, which uses comparators to sort adapteritems requires api 24

@iamareebjamal
Copy link
Member

Can you point to doc reference please

@AkshayCHD
Copy link
Contributor Author

AkshayCHD commented Mar 18, 2018

@iamareebjamal the only way to sort lists in fast adapter is as shown here, https://github.com/mikepenz/FastAdapter/blob/6b7b1cf2fe7a6d86ce8eb3ae725470d6477df356/app/src/main/java/com/mikepenz/fastadapter/app/SortActivity.java and in my opinion the already implemented way is better than that. As the method here was not added initially but was later added as in the discussion here mikepenz/FastAdapter#51 in the method we have to make a new comparator and then implement all the methods of the superclass, whereas right now sorting requires only Collection.sort() function which is simply implemented using lambda notation

@iamareebjamal
Copy link
Member

Comparator can be created as a lamda as well

@AkshayCHD
Copy link
Contributor Author

@iamareebjamal please review

@open-event-bot
Copy link

Hi @AkshayCHD!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@AkshayCHD
Copy link
Contributor Author

AkshayCHD commented Mar 20, 2018

@iamareebjamal i have added some new features to my proposal can you review them please https://docs.google.com/document/d/1pkm8zEYxfGR809NOQNMwaMsbh5T4HSmrFcf-4nDNC_w/edit?usp=sharing

@iamareebjamal
Copy link
Member

@AkshayCHD Please update the PR

@fossasia fossasia deleted a comment Mar 28, 2018
@AkshayCHD AkshayCHD force-pushed the sort branch 2 times, most recently from 791509b to fe86d2c Compare March 28, 2018 13:54
@open-event-bot
Copy link

Hi @AkshayCHD!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@open-event-bot
Copy link

Hi @AkshayCHD!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@@ -19,5 +19,15 @@
android:title="@string/filter_by_sync" />
</menu>
</item>
<item android:id="@+id/sortAttendee"
Copy link
Member

Choose a reason for hiding this comment

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

Should have an icon

@AkshayCHD
Copy link
Contributor Author

@iamareebjamal please review

@fossasia fossasia deleted a comment Mar 29, 2018
@@ -3,5 +3,5 @@
android:width="24dp"
android:viewportWidth="24"
android:viewportHeight="24">
<path android:fillColor="#000" android:pathData="M6,13H18V11H6M3,6V8H21V6M10,18H14V16H10V18Z" />
<path android:fillColor="#000" android:pathData="M3,2H21V2H21V4H20.92L14,10.92V22.91L10,18.91V10.91L3.09,4H3V2Z" />
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

@open-event-bot
Copy link

Hi @AkshayCHD!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@@ -106,6 +106,7 @@
<string name="create_event">Create Event</string>
<string name="event_name">Event Name</string>
<string name="payment_currency">Payment Currency</string>
<string name="sort_by_ticket_type">Ticket</string>
Copy link
Member

Choose a reason for hiding this comment

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

Should be ticket

default:
return super.onOptionsItemSelected(item);
}
}

void sortAttendees(int sortBy) {
Copy link
Member

Choose a reason for hiding this comment

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

Should be private

@Override
public void onDestroy() {
super.onDestroy();
fastAdapter.unregisterAdapterDataObserver(observer);
Copy link
Member

Choose a reason for hiding this comment

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

Check for nulls

@AkshayCHD
Copy link
Contributor Author

@iamareebjamal I have improved the drawable used for filtering, the new one is more relevant to the functionality provided and also is easily distinguishable from the one used for sorting, I have updated the screenshots please have a look

@@ -106,6 +106,7 @@
<string name="create_event">Create Event</string>
<string name="event_name">Event Name</string>
<string name="payment_currency">Payment Currency</string>
<string name="sort_by_ticket">Ticket</string>
Copy link
Member

Choose a reason for hiding this comment

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

Should be ticket

@AkshayCHD
Copy link
Contributor Author

@iamareebjamal please review

@fossasia fossasia deleted a comment Mar 29, 2018
@open-event-bot
Copy link

Hi @AkshayCHD!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@fossasia fossasia deleted a comment Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants