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

NT-2069:Refactor Comment Pagination code #1308

Merged
merged 2 commits into from
Jun 30, 2021
Merged

NT-2069:Refactor Comment Pagination code #1308

merged 2 commits into from
Jun 30, 2021

Conversation

hadia
Copy link
Contributor

@hadia hadia commented Jun 29, 2021

📲 What

Fix issue as on master Pagination change scrolling position and crash at the last page

🤔 Why

the compare item wasn't working as it was null for project value

👀 See

Before issue
https://user-images.githubusercontent.com/1075310/123808314-ecb57e00-d8f0-11eb-9ac1-8f234d2885cb.mp4

the Fix
https://user-images.githubusercontent.com/1075310/123807680-5d0fcf80-d8f0-11eb-9cdd-fc75a668e52d.mp4

📋 QA

Open any project with comments and has more than one page and scroll and check pagination

Story 📖

https://kickstarter.atlassian.net/browse/NT-2069

@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #1308 (f2955b5) into master (5221e67) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1308   +/-   ##
=========================================
  Coverage     74.24%   74.24%           
  Complexity      739      739           
=========================================
  Files           221      221           
  Lines          6655     6655           
  Branches        406      406           
=========================================
  Hits           4941     4941           
  Misses         1578     1578           
  Partials        136      136           

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 5221e67...f2955b5. Read the comment docs.

@@ -301,7 +305,7 @@ interface CommentsViewModel {
.distinctUntilChanged(true)
.startOverWith(startOverWith)
.envelopeToListOfData {
mapToCommentCardDataList(Pair(it, null))
mapToCommentCardDataList(Pair(it, this.project))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since project can technically be null, I think it would be best if we put a null check here to guarantee that no crash happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed map method to not allow null

Copy link
Contributor

@leighdouglas leighdouglas left a comment

Choose a reason for hiding this comment

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

lgtm! 🎉

@hadia hadia merged commit ac3c668 into master Jun 30, 2021
@hadia hadia deleted the fix_pagnagtion_error branch June 30, 2021 17:08
Arkariang added a commit that referenced this pull request Jul 6, 2021
…ure/NT-2059-update-stripe-sdk

* 'master' of github.com:kickstarter/android-oss:
  NT-1997:UI/UX – Error loading more replies (#1313)
  NT-2095:View Replies show and hide with scrolling (#1312)
  NT-2053:UX – Add pagination to replies (#1311)
  NT-2028: Navigation – Back button should slide view out #1310
  NT-1958: UX – Post a reply (#1305)
  NT-2071: Update exoplayer, mockito, and timber dependencies to latest versions (#1306)
  Set the name as an empty string to prevent the app from crashing. (#1309)
  NT-2069:Refactor Comment Pagination code  (#1308)
  NT-2005: Update Dependencies to Latest Version (#1304)
  NT-2069:Refactor Comment Pagination code (#1303)
  NT-1958: UX – Post a reply (#1301)
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.

None yet

2 participants