Offline banners for views with cached data

[itsmeichigo]

Closes #4384
⚠️ This PR depends on #4986 so please make sure that that PR is review and merged first ⚠️

Description

This PR configures offline banners for screens: My Store, Orders, Order Details, Review Order, Create Shipping Label, Products, Product Form, Reviews, Review Detail.

For simplicity, offline banner is not added to every screen, but only to critical screens that display cached data.

Changes

• Updates UIImage+Woo extension with lightning icon for the offline banner.
• Adds new extension for UIViewController to configure offline banner.
• Configure offline banner for screens: My Store, Orders, Order Details, Review Order, Create Shipping Label, Products, Product Form, Reviews, Review Detail.

Demo

RPReplay_Final1631780913.MP4

Testing

In order to test connection properly, please build these changes to a real device. Network monitor doesn't work well on simulators. Follow these steps for testing:

1. On My Store tab, turn off both wifi and cellular data if it's available on your data. Notice that the offline banner shows up.
2. Navigate to Settings, notice that offline banner doesn't show here.
3. Back to My Store and turn wifi / cellular data on. Notice that the offline banner slides away.
4. Repeat the above steps for each of above screens and their subsequent unsupported screens in the same navigation stacks.

Update release notes:

• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[Ecarrion]

Hi @itsmeichigo, thanks for working on this.

I'm a bit uncertain about this solution for the following reasons:

• It seems to be easy to misconfigure. Requires code in viewDidAppear and in viewWillDisappear.

• It is an extension on ViewController but requires the VC to be contained on a NavigationController. That dependency is not clear and I'm not sure what side effects will happen if a navigation controller does not exist 🤔

• As the network observer is a singleton I don't think this will scale well for multiple windows on iPad or a split view.

• I'm not sure how this will play for SwiftUI views. I guess we will have to add this code on the HostingController 🤔

• I think we lose the ability of a view controller to set its own toolbar content, but maybe we should not worry about this as it hasn't been needed yet.

---

As an alternative for most of these scenarios, what do you think of this approach?

As your approach requires a navigation controller and we already have a WooNavigationController subclass. We could define a property like:

extension UIViewController {
    /// Defines if the view controller should show a "no connection" banner when offline.
    /// This requires the view controller to be contained inside a `WooNavigationController`
    /// Defaults to `false`
    ///
    @objc func shouldShowNoNetworkIndicator() -> Bool {
        false
    }
}

Then on the navigation delegate, you could do something like

func navigationController(_ navigationController: UINavigationController, willShow viewController: UIViewController, animated: Bool) {
    if viewController.shouldShowNoNetworkIndicator() && connectionObserver.isConnectivityAvailable {
        // show banner on toolbar or in a separate view(to keep toolbar usable for the future)
    } else {
        // hide toolbar or view
    }
}

Lastly, we could have the network observer as an instance property of WooNavigationController instead of having it as a singleton. This will allow us for this solution to scale more multiple windows or split views and we will have one less singleton to worry about.

What do you think?

[Ecarrion]

Testing it on an iPad I believe there is a problem when following these steps

• Launch the app, turn off wifi, see the banner, kill the app.
• Turn on wifi
• Launch the app
• Se that the banner is visible until I navigate from tab to tab.

RPReplay_Final1631801017.MP4

[itsmeichigo]

Thanks so much @Ecarrion for the thorough review!

I really like the idea of letting view controller decides whether to support offline banner, and using willShow in navigation controller delegate is just brilliant 💯. I applied these suggestions in my update to avoid misconfiguration as you pointed out.

As for these concerns:

As the network observer is a singleton I don't think this will scale well for multiple windows on iPad or a split view.

I don't think singleton is the issue - in fact I'd prefer to request to observe network status only once because there's no point in doing that multiple times, the result is just the same. But the use of 1:1 observer pattern in updateListener method sure can cause trouble in iPad split view! I think it's better to use 1-to-many pattern - so I added statusPublisher so that any view controller can observe this and update its UI. (I'm not sure if I should remove updateListener then).

I'm not sure how this will play for SwiftUI views. I guess we will have to add this code on the HostingController 🤔

I don't think setting up the banner in hosting controller can work, since we can have different views inside of one hosting controller. Since updating UI in SwiftUI is easier, I think it's best that we handle each view separately. I'm thinking about rendering a reusable offline banner based on the connectivity status, but there are 2 other problems:

• We're probably better off using DefaultConnectivityObserver directly to make use of the @Published variable in SwiftUI view. Should we make DefaultConnectivityObserver a singleton or should we create one in each view? I'm still not sure about the cost of the creation.
• Since we're creating the banner manually, there's no need to use the hacky toolbar. But UI may look inconsistent with UIKit screens.

I think we lose the ability of a view controller to set its own toolbar content, but maybe we should not worry about this as it hasn't been needed yet.

This I agree - I'm not sure yet how to add normal view into all these screens without too much effort.

I think this PR is ready for another look, please let me know what you think of the new changes!

[itsmeichigo]

@Ecarrion as for the issue you found on the iPad - I believe that it takes a while for the network monitor to properly update new status. I reproduced the same issue on my device, but after waiting for several seconds the banner did slide away.

[Ecarrion]

Hi @itsmeichigo, thanks for taking my comments into consideration!

I think however that there is an opportunity to continue simplifying this.

---

I think that if you add only one network listener to WooNavigationControllerDelegate then you can share that state between all view controllers that define

func shouldShowNotNetworkIndicator() -> {
    true
}

Then you can drop the extra complexity of having to set up an observation and a cancelable in each view controller.

---

Regarding the other concerns.

This I agree - I'm not sure yet how to add normal view into all these screens without too much effort.

I think you can add the view as a regular subview to the navigation controller and then modify the presented view controller safe area like

viewController.additionalSafeAreaInsets = //banner size

I believe that it takes a while for the network monitor to properly update new status. I reproduced the same issue on my device, but after waiting for several seconds the banner did slide away.

I think this is quite concerning 😟

[itsmeichigo]

@Ecarrion Thanks again for the comments, I really appreciate it!

I did think about using additionalSafeAreaInsets before, but this property, as its name suggests, doesn't support removing the previously added insets. So in situation when we want to remove the offline banner, using a negative value for additionalSafeAreaInsets just doesn't work, and we're left with an empty space instead.

I have added a few more changes as suggested so this PR is ready for another look!

[Ecarrion]

Hey @itsmeichigo,

Probably I didn't explain myself correctly, but this is what I was proposing for simplifying the setup on each VC & maintaining state only in the WooNavigationControllerDelegate.

Let me know what you think!

I did think about using additionalSafeAreaInsets before, but this property, as its name suggests, doesn't support removing the previously added insets. So in situation when we want to remove the offline banner, using a negative value for additionalSafeAreaInsets just doesn't work, and we're left with an empty space instead.

I would have thought that you could do

additionalSafeAreaInsets = .zero

But I haven't validated it completely.

[itsmeichigo]

Thanks @Ecarrion for the proposed changes, it makes much more sense to handle the banner configuration in the navigation controller delegate instead of in each individual view controllers.

And using .zero for additionalSafeAreaInsets indeed helps with removing the additional space. Thanks for pointing that out!

So I have made more changes - this time I replaced the usage of the navigation controller tool bar with adding the banner as subview of the view controller's view. I think it looks much cleaner now, but not sure if it's still relevant to keep the banner configuration in the navigation controller delegate. It's nice that we don't have to repeat the logic in every view controller though, WDYT?

[Ecarrion]

This works and looks super simple to configure!

As for my suggestions:

This is what I mean on adding the view to the navigation controller. It will allow us to have only one view, have the configuration without messing with the VC internals, and save us from looking for the offline banner using loops.

---

I'm more concerned about the lack of accessibility support for it. But feel free to create an issue and tackle it later.

[Ecarrion]

If we have the banner as a lazy var we could later do bannerView.superView == nil right?
I'm thinking that we could add the view to the navigation view but still modify the additionalSafeAreaInsets of the VC.

Would that work?

[itsmeichigo]

I like the idea of reusing the banner view, so I've tried your suggested solution but found a couple of issues:

• Calling removeConstraints on all constraints of the banner causes the banner to lose even its constraints to its sub views (the image and title inside). So we may need to keep references to the constraints to parent view to remove them later.
• Handling the constraints is tedious, so I thought about only adding the banner once and then showing / hiding it later. This introduces another issue about updating bottom constraint for view controller with / without the bottom bar. It's still necessary to keep reference to this bottom constraint.
• Adding banner on navigation controller can also cause issue when swiping the navigation controller half way through before releasing:
  https://user-images.githubusercontent.com/5533851/134460006-b8b7ed0a-a831-4ed0-84a0-0a84a6adf26d.MP4

So I'll keep the current solution until finding a better one.

[Ecarrion]

I don't think this is safe to do, would it all fit correctly if fonts grow bigger?

Probably better to get that height dynamically with systemLayoutFittingSize

[itsmeichigo]

I have created a separate issue here #5041 to handle it later.

[itsmeichigo]

Thank you a lot @Ecarrion for the discussions! I'll go ahead and merge this and handle #5041 in another PR.