Page MenuHomePhabricator

[web] removed pagination logic from thread picker
ClosedPublic

Authored by ginsu on Sep 26 2022, 11:00 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 21, 9:08 AM
Unknown Object (File)
Wed, Mar 20, 6:40 PM
Unknown Object (File)
Wed, Mar 20, 6:40 PM
Unknown Object (File)
Wed, Mar 20, 6:39 PM
Unknown Object (File)
Wed, Mar 20, 6:39 PM
Unknown Object (File)
Wed, Mar 20, 6:39 PM
Unknown Object (File)
Wed, Mar 20, 6:39 PM
Unknown Object (File)
Wed, Mar 20, 6:39 PM

Details

Summary

removed pagination logic as discussed in Linear. Now if there are more than 5 threads, they will all just render instead of being paginated

Next steps:

  1. Implement the search logic

Linear task: ENG-1847

Test Plan

Please view the attached screenshots to see the before and after of the changes I made:

Before:

Screen Shot 2022-09-26 at 1.47.07 PM.png (462×556 px, 42 KB)

6 threads would cause the picker to paginate and show the first 5 threds

After:

Screen Shot 2022-09-26 at 1.59.21 PM.png (412×454 px, 39 KB)

Now all 6 threads are shown

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added a reviewer: atul.
ginsu edited the summary of this revision. (Show Details)

removed current page state

atul requested changes to this revision.Sep 26 2022, 6:22 PM

At a high level this looks good, but let's go through this component one more time and double check that everything related to pagination has been removed.

web/calendar/thread-picker.react.js
43 ↗(On Diff #17089)

Is there a reason we need to keep this type around given the contents are empty?

46 ↗(On Diff #17089)

Is pageSize still being used anywhere?

This revision now requires changes to proceed.Sep 26 2022, 6:22 PM

Looks good to me, I took a look at the new revision based off of the previous comments and pulled the diff down locally to test it. Works as expected on my local environment

atul requested changes to this revision.Sep 28 2022, 7:47 AM

Looks super close, let's remove LeftPager and RightPager in this diff as well.. couldn't find any other usage in the codebase.

web/calendar/thread-picker.react.js
11 ↗(On Diff #17111)

Are LeftPager or RightPager used elsewhere in the codebase?

If not, let's remove those as well in this diff

This revision now requires changes to proceed.Sep 28 2022, 7:47 AM

removed pager vectors and pager css

Sweet! Nice catching those no-longer-relevant CSS selectors as well

This revision is now accepted and ready to land.Sep 28 2022, 11:14 AM