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

NT-1951: UI/UX – Error loading initial comments #1284

Merged
merged 14 commits into from
Jun 11, 2021
Merged

NT-1951: UI/UX – Error loading initial comments #1284

merged 14 commits into from
Jun 11, 2021

Conversation

sunday-okpoluaefe
Copy link
Contributor

πŸ“² What

UI/UX – Error loading initial comments

πŸ€” Why

  • Because every now and then things will fail, we need to handle problems loading comments.

πŸ›  How

  • Created custom error layout item
  • Added custom item to viewholder
  • Added Initial Load Error observable
  • Subscribed to initialCommentLoadError
  • Added unit tests for initial comment load error

πŸ‘€ See

initial.error.mp4
Before πŸ› After πŸ¦‹

πŸ“‹ QA

  • Select any project with comments
  • Turn off network connect
  • Select comments, you should see an error page on failure

Story πŸ“–

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

@codecov
Copy link

codecov bot commented Jun 10, 2021 •
edited
Loading

Codecov Report

Merging #1284 (afbeb82) into master (4f9dd71) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1284   +/-   ##
=========================================
  Coverage     74.34%   74.34%           
  Complexity      739      739           
=========================================
  Files           221      221           
  Lines          6646     6646           
  Branches        405      405           
=========================================
  Hits           4941     4941           
  Misses         1569     1569           
  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 4f9dd71...afbeb82. Read the comment docs.

Arkariang
Arkariang reviewed Jun 10, 2021
View reviewed changes
app/src/main/java/com/kickstarter/libs/utils/extensions/BoolenExt.kt

import android.view.View

fun Boolean.toVisibility(): Int {
Copy link
Contributor

Choose a reason for hiding this comment

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

love it! ❀️

@sunday-okpoluaefe sunday-okpoluaefe marked this pull request as ready for review June 10, 2021 21:13
Arkariang
Arkariang reviewed Jun 10, 2021
View reviewed changes
app/src/main/java/com/kickstarter/ui/adapters/CommentsAdapter.kt
}

fun takeData(comments: List<CommentCardData>) {
resetList()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not call resetList(), that should be only initialization step

Arkariang
Arkariang reviewed Jun 10, 2021
View reviewed changes
app/src/main/java/com/kickstarter/ui/adapters/CommentsAdapter.kt
}

fun insertPageError() {
resetList()
Copy link
Contributor

Choose a reason for hiding this comment

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

same as before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing resetList causes some weird behavior. The previous error page persists even after data arrives

Copy link
Contributor

@Arkariang Arkariang Jun 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

fyi: you need to clean specific sections not all of them, so it's the expected behaviour. Meaning if you need to show the section "SECTION_INITIAL_LOAD_ERROR" send an empty list to the section "SECTION_COMMENT_CARD" and viceversa. For now it's not a problem but I'm gonna have to change it either way on my open PR(#1284) as I need to present there 2 sections at the same time

Arkariang
Arkariang approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@Arkariang Arkariang left a comment

Choose a reason for hiding this comment

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

I left a few comments , nothing serious just small tweecks.
Once those comments are addressed feel free to merge, overall it looks great :) πŸ‘

@sunday-okpoluaefe sunday-okpoluaefe merged commit 3737f82 into master Jun 11, 2021
@sunday-okpoluaefe sunday-okpoluaefe deleted the sunday-nt-1951-error-loading-initial-comments branch June 11, 2021 17:02
Arkariang added a commit that referenced this pull request Jun 11, 2021
@Arkariang
Merge branch 'master' of github.com:kickstarter/android-oss into imar…
4e8da11 
…tin/NT-2037-inside-list-error

* 'master' of github.com:kickstarter/android-oss:
  NT-1951: UI/UX – Error loading initial comments (#1284)

# Conflicts:
#	app/src/main/java/com/kickstarter/ui/adapters/CommentsAdapter.kt
#	app/src/main/java/com/kickstarter/viewmodels/CommentsViewModel.kt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arkariang Arkariang Arkariang approved these changes

@hadia hadia Awaiting requested review from hadia

@leighdouglas leighdouglas Awaiting requested review from leighdouglas

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

Successfully merging this pull request may close these issues.
2 participants
@sunday-okpoluaefe @Arkariang