Skip to content

Conversation

chickdan
Copy link
Contributor

@chickdan chickdan commented Dec 4, 2018

Had some code laying around and plugged it in to get network detection going and added the NotificationBanner library for displaying a banner. Currently the only banner being displayed is when there is no network connection at all, but I plan on expanding the logic to display for various situations with corresponding messages.

This code is kind of old so I'm looking at using the Reachability.swift library which looks pretty similar but more refined.

Closes #103

@benhalpern
Copy link
Contributor

This looks great. I'll take a look within the next day. @StriderHND wanna play around with this and offer some thoughts?

@StriderHND
Copy link
Contributor

Sure I'll checkout this @benhalpern , and give my comments.

@chickdan
Copy link
Contributor Author

chickdan commented Dec 6, 2018

In 8681c0c I added an error banner as a lazy var, this is to provide access the banner to dismiss it since autoDismiss = false (a little more info here on how the library works in this case). Along with that there are two new banners that will conditionally display if moving from no connection to being connected to either cellular or WiFi.

I'll be tackling the code climate issues soon.

@StriderHND
Copy link
Contributor

@chickdan the changes are looking good! I was gonna mention the issue with the autoDismiss but already looked into that!

Just fix the code climate issues and it's good to go. @benhalpern

@chickdan chickdan changed the title [WIP] Network Reachability Network Reachability Dec 7, 2018
@benhalpern
Copy link
Contributor

Having finally given it a full inspection, this is a fabulous first pass.

This is an area we are going to want to keep improving on as a strength and this is an awesome start to iterate off. Amazing work @chickdan.

@benhalpern benhalpern merged commit e85d91a into forem:master Dec 9, 2018
@chickdan chickdan deleted the network_reachability branch March 22, 2019 03:26
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.

3 participants