wordpress-mobile / WordPress-iOS

WordPress for iOS - Official repository
http://ios.wordpress.org/
GNU General Public License v2.0
3.7k stars 1.12k forks source link

Reader: NoResultsView is blank in Reader detail while offline #14410

Closed designsimply closed 3 months ago

designsimply commented 4 years ago

Steps to reproduce:

  1. Go to the main stream in the Reader.
  2. Turn on Airplane mode.
  3. Tap any post to open the Reader detail.

Result: The no results view for the body of Reader detail is blank if I turn on airplane mode first and then start browsing. I expected to see a message saying “Unable to load this content right now” in the content area if content cannot be loaded while offline. See 14283.

IMG_4338

Tested with WPiOS 15.2.0.0 TestFlight beta on iPhone 11 iOS 13.5.1.

leandroalonso commented 4 years ago

So, turns out that is an easy fix but I have a few questions before submitting the PR. The previous Reader behaved the same way: it displayed the content even if the user is offline (a few exceptions are the x-posts or when you try to load a post that we don't have the text content pre-cache).

Of course, this approach has it's own problems: images don't appear, for example. But at least the user has something to read.

So I'm really divided here: should we show the content (even if it appears broken) or don't?

@osullivanchris since you were part of the Offline Posts project, what are your thoughts? @yaelirub?

yaelirub commented 4 years ago

I think we should try to display the content AND show a notice saying something like "No connectivity. Some content might not display correctly" obviously this needs some editorial love :P

osullivanchris commented 4 years ago

Hey! Thanks for the ping @leandroalonso

Firstly, I am struggling to reproduce. It seems like any post which I can load in my stream, also has the content when I drill into the post.

So I agree with Yael. If we have the headline and image loaded, but not the body, it seems unfriendly to the user to show an empty state for everything...something is better than nothing.

I haven't been able to reproduce to see, but it probably needs a loading state and an error state. Here's my first attempt. I looked at what we already have...first the skeleton loading state we have in the reader filter sheet. And the error state we have if you're offline in the reader feed. I just put a skeleton loading state in the body area, and error in same place

content not loaded

It kinda looks like a full page error state though, and not necessarily specific to the body. So we could make that error state smaller

content not loaded small

Then I'm wondering how this would work if everything failed to load not just the body. Here, everything has a skeleton loading state. Then a larger error.

nothing loaded

I don't know how complex it is to show loading states and error states for different parts of the view, rather than the whole view. First thoughts anyway so just let me know what you think!

leandroalonso commented 4 years ago

@osullivanchris I wasn't able to reproduce the same issue @designsimply shared.

For me, the content was always displaying too. The main issue here is that if you haven't saved the post to read later, images won't appear.

osullivanchris commented 4 years ago

Sorry I didn't see a mention of images @leandroalonso . But for images, if this approach works, we could do the same. Show a box for a loading state. And show an error if the image doesn't load.

image didnt load

I don't know what error states we already have covered, and also whether its possible to 'know' whats in the post, without being able to load it, to mark these smaller pieces of unloaded content.

leandroalonso commented 4 years ago

@osullivanchris I like this approach of displaying this placeholder with this message. What do you think @yaelirub?

In that case, if a post only contains text it will appear 100% correct and without any message stating that the user is offline. I guess this matches our principles.

yaelirub commented 4 years ago

SGTM, @leandroalonso ! Thanks so much for the quick turnaround, @osullivanchris!

osullivanchris commented 4 years ago

No problem, let me know if you want more input or to review any changes!

rachelmcr commented 4 years ago

Will this fix also resolve the similar issue reported in https://github.com/wordpress-mobile/WordPress-iOS/issues/12517? (Just checking if we can close two issues with one fix, no problem if not.)

leandroalonso commented 4 years ago

@rachelmcr I'd say yes. If the image fails to load, we'll clearly communicate the issue.

osullivanchris commented 4 years ago

Woah looks like @mattmiklic mocked something very similar to what I was doing 😄 We should use his mocks for the image issue here too, its solving the exact same thing...and pretty much exactly what I was suggesting anyway.

yaelirub commented 4 years ago

Excellent, @osullivanchris ! Where can we find the mocks?

leandroalonso commented 4 years ago

@yaelirub it's on the other issue mentioned by @rachelmcr (here)

designsimply commented 3 years ago

Tested with WPiOS 17.7.0.3 (TestFlight beta) and found that images in reader details screens are still not loading or showing an image not loading notice when viewing an individual post while offline (this happens even if the content was previously loaded in the same session. This video is long (apologies!) but illustrates the problem pretty well too: 6m29s.

image

In terms of prioritization, I'd like to downgrade this from blocking to high priority and ask for it to be considered as something to add to the next project that touches the Reader (if it makes sense there). @leandroalonso can you help me get it assigned or added to a future project?

leandroalonso commented 3 years ago

@designsimply sure!

Just so I can understand more, I want to clarify the Pri [High], by looking at our data usage it seems that almost 0% of users open reader articles while offline. Data from the last 14d:

Would you still consider it high priority?

jkmassel commented 3 months ago

Closing, as this is an issue that doesn't seem to affect anyone