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.

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

Feature 115 loading spinner #118

Merged
merged 4 commits into from Jan 8, 2019

Conversation

StriderHND
Copy link
Contributor

This PR resolves the Feature Request in #115 adding a Loading Spinner for the first time a app opens or when the app opens from a push notification

@StriderHND StriderHND changed the title Feature 155 loading spinner Feature 115 loading spinner Jan 3, 2019
Copy link
Contributor

@benhalpern benhalpern left a comment

Choose a reason for hiding this comment

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

When I visit an outside link, the spinner is not visible (which would be not so bad), but then when I close that popup, the spinner persists on the original view.

Change needed: either the spinner needs to work properly in this use case or not be triggered in this case. 🙂

@StriderHND
Copy link
Contributor Author

@benhalpern ok let me check that right now

@benhalpern
Copy link
Contributor

benhalpern commented Jan 5, 2019

Here's a gif of the issue

Spinner reproduction

@StriderHND
Copy link
Contributor Author

Hello Ben I've tried to look at the gif but looks like has a expiration date. now is a 404.

@benhalpern
Copy link
Contributor

@StriderHND Hey sorry, this should work: https://imgur.com/a/7XrpVdq

@StriderHND
Copy link
Contributor Author

@StriderHND Hey sorry, this should work: https://imgur.com/a/7XrpVdq

Perfect!, let me check this right away.

…dInBrowserView to use instead navigation segue functions.
@StriderHND
Copy link
Contributor Author

@benhalpern I've corrected the issues and made some more refectory on the function that was presenting the external links, I made a change to use segues instead of making a direct instance from the storyboard that way we preserve hierarchy on the views.

@@ -187,7 +193,7 @@ class ViewController: UIViewController, WKNavigationDelegate {
} else if isAuthLink(url) {
return .allow
} else if url.host != "dev.to" && navigationType.rawValue == 0 {
loadInBrowserView(url: url)
performSegue(withIdentifier: "showExternalPage", sender: url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you pull out this hard coded string to a global variable? Same with line 305

Copy link
Contributor Author

@StriderHND StriderHND Jan 7, 2019

Choose a reason for hiding this comment

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

If we are going to start to add global variables we should add a struct for this.

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.

None yet

3 participants