feat: Add Pagination to Members Page #87

techno-disaster · 2020-05-27T20:43:23Z

Description

Add Pagination to load the members list on members page

Fixes #85

Flutter Channel:

I have used the Flutter Beta channel on my local machine

Type of Change:

Delete irrelevant options.

Code

Code/Quality Assurance Only

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Physical Device.
Steps :
Go to members page and scroll down too see if pagination works as required.

Checklist:

Delete irrelevant options.

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials
I have commented my code or provided relevant documentation, particularly in hard-to-understand areas

Code/Quality Assurance Only

My changes generate no new warnings

bartekpacia

Wow, great!

bartekpacia · 2020-05-27T20:52:58Z

lib/remote/services/auth_service.chopper.dart

@@ -6,6 +6,7 @@ part of 'auth_service.dart';
 // ChopperGenerator
 // **************************************************************************

+// ignore_for_file: always_put_control_body_on_new_line, always_specify_types, prefer_const_declarations


Please don't include generated files under git.

I approved because it's a small change but please do not forget about it.

We haven't made those changes in the documentation though. So I don't think we can remove it yet in case a new contributor comes...
What do you think @bartekpacia ?

@bartekpacia when you need a change request you might as well, request change. because once @techno-disaster changes, you review will be dismissed

techno-disaster · 2020-05-27T20:55:58Z

Wow, great!

i did not expect a straight away approve lol, no need to change variable names or something? xd Thanks @bartekpacia 👍

techno-disaster · 2020-05-27T22:48:32Z

@anitab-org/qa-team can anyone test this?
Steps :
Go to members page and scroll down too see if pagination works as required.

isabelcosta · 2020-05-27T23:03:58Z

@anitab-org/qa-team can anyone test this?
Steps :
Go to members page and scroll down too see if pagination works as required.

The steps you mention here can be mentioned on the how has this been tested section in your PR description.

techno-disaster · 2020-05-27T23:26:34Z

@anitab-org/qa-team can anyone test this?
Steps :
Go to members page and scroll down too see if pagination works as required.

The steps you mention here can be mentioned on the how has this been tested section in your PR description.

ok. thanks

Akanksha1212 · 2020-05-29T06:07:58Z

Not from QA team just tested this out of curiosity. Works perfectly fine on my phone. Great Work! @techno-disaster

bartekpacia · 2020-05-29T23:39:52Z

@techno-disaster What about increasing the number of paged records? Currently it seems to have to be reloaded a bit too often.

techno-disaster · 2020-05-29T23:40:51Z

@techno-disaster What about increasing the number of paged records? Currently it seems to have to be reloaded a bit too often.

you mean increasing the number of members we get on screen each load?

bartekpacia · 2020-05-29T23:42:20Z

Exactly.

techno-disaster · 2020-05-29T23:53:51Z

Exactly.

Thats just hardcoded to 10, are you saying we should increase it? 🤔

techno-disaster · 2020-05-30T01:36:34Z

@isabelcosta what do you think about this? how many members should be loaded at once?

isabelcosta · 2020-05-30T01:38:02Z

@techno-disaster I have no idea what the criteria should be for deciding this number. @bartekpacia any idea? If in doubt, we can ask for help. @annabauza mind to give your opinion here?

techno-disaster · 2020-05-30T01:39:58Z

We could give the user the option, but considering what kind of app this is and what its purpose is, it feels a bit irrelevant also giving users technical options would confuse them i think

techno-disaster · 2020-05-30T01:41:15Z

Loading more number of users would obviously need more time to load. and the app kinda already feels slow, IMHO 10 is a good number. 15 looks good too

bartekpacia · 2020-05-30T15:19:55Z

I'd load 20-25. The app will get faster in the future as we'll improve it. Fetching just 10 users doesn't solve the problem of slowness.

techno-disaster · 2020-05-30T16:07:34Z

Here's how 30 looks. IMO 30 looks a bit slow cc: @isabelcosta @bartekpacia @annabauza

techno-disaster · 2020-06-01T21:03:02Z

@isabelcosta @bartekpacia @annabauza any updates? I attached a load 30 members per page video above for refrence

isabelcosta · 2020-06-01T23:10:48Z

@bartekpacia @techno-disaster My vote goes to 20, its a middle between 10 (too fast) and 30 (too slow).
I don't know exactly what would be the best criteria here, but based on your points I believe that is a good choice.

techno-disaster · 2020-06-02T01:27:36Z

@bartekpacia @techno-disaster My vote goes to 20, its a middle between 10 (too fast) and 30 (too slow).
I don't know exactly what would be the best criteria here, but based on your points I believe that is a good choice.

I am good with 20 too :) Waiting for one more opinion, will update PR then

techno-disaster · 2020-06-03T19:33:31Z

Small update, I was running this on the debug app version which is slower than the actual release apk. @bartekpacia what do you think we should go with?

bartekpacia · 2020-06-03T19:47:22Z

Let's stick with 20 :)

ksorv · 2020-06-04T01:11:15Z

Hey I'd love to gove you guys some input on this, this is kinda wrong way to do it!

Why dont you guys do something like:

fetchUsers(query, limit, skip) {
    // Returns limit no of users, searched over the db with query and for next one call to fetchUsers, it skips the limit you provided cause the objects or whatever returned from db doesn't change that fast!

I've been using this, its a gud pattern i guess!

It lets me define different limits for different things, like when i want to search for people i put limit to null and Put data returned in an infinite scroll.

For when a user search in like a emailing box, it shows like 10-20 or so.

What you guys say?

techno-disaster · 2020-06-04T04:52:46Z

Hey I'd love to gove you guys some input on this, this is kinda wrong way to do it!

Why dont you guys do something like:
fetchUsers(query, limit, skip) {
    // Returns limit no of users, searched over the db with query and for next one call to fetchUsers, it skips the limit you provided cause the objects or whatever returned from db doesn't change that fast!
I've been using this, its a gud pattern i guess!

It lets me define different limits for different things, like when i want to search for people i put limit to null and Put data returned in an infinite scroll.

For when a user search in like a emailing box, it shows like 10-20 or so.

What you guys say?

Currently we only have a /users/verified endpoint from which we get the list and don't need it anywhere else apart from the members page. Also the backend team plans to change this soon, they are planning to change how users are sent, I'm not sure though and this might change how we fetch user list on the app, so I think we can write this part after thats done. .Maybe @isabelcosta could help you with this. Thanks for your update thought 👍😀

techno-disaster · 2020-06-04T10:24:51Z

Updated code, app now loads 20 members per page.

bartekpacia

looks good :)

…ip-flutter into issue-85-add-pagination

isabelcosta · 2020-06-10T22:03:54Z

@anitab-org/qa-team can anyone test this PR, so we can consider it "Ready to Merge" if all goes well :)

bartekpacia · 2020-06-10T22:29:37Z

@isabelcosta I bet you meant test ;P

isabelcosta · 2020-06-10T22:34:14Z

@bartekpacia you correct! I just edited this :P

foongminwong

The changes made in this PR were tested locally. Following are the results:

Code review - Done
All possible responses were tested as below:
- Before PR 87
  Screenshot/gif/url:
Expected Result: The Members list cannot be scrolled fully. It'll only show the first 10 users.
Actual Result: Same as expected.
- After PR 87
Screenshot/gif/url:
Expected Result: The Members list can be scrolled fully and we can navigate the whole list now.
Actual Result: Same as expected.
Additional testcases covered: N/A
Additional Comments: N/A
Status of PR Changed to: Changed from Needs Review to Ready to Merge
Android Version: 9.0, Device: Android Emulator Pixel API 28

techno-disaster · 2020-06-10T23:10:42Z

@isabelcosta looks like its ready to be merged 👀

isabelcosta · 2020-06-11T20:10:13Z

Awesome @techno-disaster and @foongminwong 👏

isabelcosta

Great contribution @techno-disaster 👏 🎉

Techno-Disaster added 3 commits May 27, 2020 13:54

i hate bloc ffs

72aba3b

done ig? cleanup soon

d635795

feat: add pagination

fdb43fb

techno-disaster requested review from bartekpacia and isabelcosta as code owners May 27, 2020 20:43

bartekpacia previously approved these changes May 27, 2020

View reviewed changes

techno-disaster added Category: Coding Changes to code base or refactored code that doesn't fix a bug. Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. labels May 27, 2020

techno-disaster mentioned this pull request May 30, 2020

MembersPage: strange FormatException #36

Closed

rpattath added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. labels Jun 2, 2020

set per page members to 20

b94cad2

techno-disaster dismissed bartekpacia’s stale review via b94cad2 June 4, 2020 10:18

techno-disaster requested a review from bartekpacia June 4, 2020 10:25

bartekpacia approved these changes Jun 4, 2020

View reviewed changes

Merge branch 'develop' of https://github.com/Techno-Disaster/mentorsh…

3dc76a1

…ip-flutter into issue-85-add-pagination

foongminwong self-requested a review June 10, 2020 22:42

foongminwong approved these changes Jun 10, 2020

View reviewed changes

foongminwong added Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Jun 10, 2020

isabelcosta approved these changes Jun 11, 2020

View reviewed changes

isabelcosta merged commit 2b3568d into anitab-org:develop Jun 11, 2020

techno-disaster deleted the issue-85-add-pagination branch June 16, 2020 01:51

feat: Add Pagination to Members Page #87

feat: Add Pagination to Members Page #87

Conversation

techno-disaster commented May 27, 2020 • edited

Description

Flutter Channel:

Type of Change:

How Has This Been Tested?

Checklist:

bartekpacia left a comment

Choose a reason for hiding this comment

bartekpacia May 27, 2020

Choose a reason for hiding this comment

bartekpacia May 27, 2020

Choose a reason for hiding this comment

techno-disaster May 27, 2020

Choose a reason for hiding this comment

isabelcosta May 27, 2020

Choose a reason for hiding this comment

techno-disaster commented May 27, 2020

techno-disaster commented May 27, 2020

isabelcosta commented May 27, 2020

techno-disaster commented May 27, 2020

Akanksha1212 commented May 29, 2020

bartekpacia commented May 29, 2020

techno-disaster commented May 29, 2020

bartekpacia commented May 29, 2020

techno-disaster commented May 29, 2020 • edited

techno-disaster commented May 30, 2020

isabelcosta commented May 30, 2020

techno-disaster commented May 30, 2020

techno-disaster commented May 30, 2020

bartekpacia commented May 30, 2020

techno-disaster commented May 30, 2020

techno-disaster commented Jun 1, 2020 • edited

isabelcosta commented Jun 1, 2020

techno-disaster commented Jun 2, 2020 • edited

techno-disaster commented Jun 3, 2020

bartekpacia commented Jun 3, 2020

ksorv commented Jun 4, 2020

techno-disaster commented Jun 4, 2020

techno-disaster commented Jun 4, 2020 • edited

bartekpacia left a comment

Choose a reason for hiding this comment

isabelcosta commented Jun 10, 2020 • edited

bartekpacia commented Jun 10, 2020

isabelcosta commented Jun 10, 2020

foongminwong left a comment • edited

Choose a reason for hiding this comment

techno-disaster commented Jun 10, 2020

isabelcosta commented Jun 11, 2020

isabelcosta left a comment

Choose a reason for hiding this comment

techno-disaster commented May 27, 2020 •

edited

techno-disaster commented May 29, 2020 •

edited

techno-disaster commented Jun 1, 2020 •

edited

techno-disaster commented Jun 2, 2020 •

edited

techno-disaster commented Jun 4, 2020 •

edited

isabelcosta commented Jun 10, 2020 •

edited

foongminwong left a comment •

edited