Skip to content
This repository has been archived by the owner on Jul 23, 2024. It is now read-only.

Ema 276 library pull to refresh #285

Merged
merged 2 commits into from
Sep 27, 2020

Conversation

filbabic
Copy link
Contributor

Added a Pull to refresh component, and some logic to fix failed requests.

Hopefully fixes #276.

I noticed an issue with loading the Library data. For me (device & emulator), I run the app, and the Library loading progress just keeps on spinning. If I don't switch to a different fragment and back, it just never stops spinning, and never loads the data.

Here's a video describing the behavior: https://www.dropbox.com/s/d2wz0sh86yqjcpp/continuous%20loading.webm?dl=0

I left my phone in the loading state to see if it'll work out, but after over 10 minutes of loading, nothing happened.

By adding some smaller changes such as:

OkHttpClient.Builder()
  .connectTimeout(30, TimeUnit.SECONDS)
  .callTimeout(30, TimeUnit.SECONDS)
  .readTimeout(30, TimeUnit.SECONDS)
  .writeTimeout(30, TimeUnit.SECONDS)

In the NetModule (fixes the issue), and

      UiStateManager.UiState.INIT_FAILED -> {
        loadCollections() // retry
      }

within the LibraryFragment UI handling, I manage to add some sort of a fallback, that just loads the data continuously until the data is actually there. I also noticed we don't have proper error handling to mitigate this, so the spinner just keeps on spinning in case we don't manage to load the data properly. Not sure if this is wanted behavior @orionthewake.

@sammyd Could you check this endpoint, to see if there's something iffy with the server/api?

Endpoint: https://api.raywenderlich.com/api/contents?page%5Bnumber%5D=1&page%5Bsize%5D=10&filter%5Bcontent_types%5D%5B%5D=collection&filter%5Bcontent_types%5D%5B%5D=screencast&filter%5Bq%5D=&sort=-released_at

This is pure console output, so if you need specific data pre-formatting/encoding, let me know.

I basically realized that the request takes over 15 seconds, and the default timeout was 10ish, so adding larger timeouts should fix the issue, but I'd love it if we could see if we could optimize the request somehow.

--

The behavior regarding the Pull To Request feature is shown below in the video:

Pull To Refresh: https://www.dropbox.com/s/mg9huokv5tcndbv/pull%20to%20refresh.webm?dl=0

It kind of falls out of place regarding our design system, as it's just a regular PTR, but I think it's a good fallback for folks that wanted this feature. Let me know if we'd like to apply more design to this, or change it up somehow!

--

I do have some concerns regarding our current loading process, and the way data is being handled/propagated throughout the app, I think it may be a bit too complex for the simplicity of what it does (loads 1-2 requests and that's it), so I'd love to see if I can try to optimize this or clean up the code a bit in the future. But also, it works fine, so I guess "if it ain't broke, don't fix it"? @orionthewake Would love to hear your thoughts.

@filbabic filbabic added bug Something isn't working enhancement New feature or request labels Sep 16, 2020
@filbabic filbabic self-assigned this Sep 16, 2020
@orionthewake orionthewake merged commit 276e323 into development Sep 27, 2020
@orionthewake orionthewake deleted the EMA-276-library_pull_to_refresh branch September 27, 2020 23:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library Pull to Refresh
2 participants