Skip to content

NavigateAndLoad: Cancel loading on dismiss #757

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 1, 2021
Merged

NavigateAndLoad: Cancel loading on dismiss #757

merged 5 commits into from
Sep 1, 2021

Conversation

@filblue
Copy link
Contributor

@filblue filblue commented Sep 1, 2021

No description provided.

stephencelis
stephencelis reviewed Sep 1, 2021
View reviewed changes
Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! We're definitely down to adding cancellation to case studies that fire off effects like this. Just one note on preferring delay to deferred in these cases.

Would you like to add cancellation to the other loadable studies as well?

filblue and others added 3 commits September 1, 2021 21:24
@filblue @stephencelis
Update Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Navigate…
d60f35c 
…AndLoad.swift

Co-authored-by: Stephen Celis <stephen.celis@gmail.com>
@filblue @stephencelis
Update Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Sheet-Pr…
460a49d 
…esentAndLoad.swift

Co-authored-by: Stephen Celis <stephen.celis@gmail.com>
@filblue
Add cancellation to UIKitCaseStudies/NavigateAndLoad.swift
3fedb31
@filblue
Copy link
Contributor Author

filblue commented Sep 1, 2021

@stephencelis I've also added cancellation to UIKitCaseStudies/NavigateAndLoad.swift. I believe LoadThenNavigate ones don't need this, right?

@stephencelis
Copy link
Member

stephencelis commented Sep 1, 2021

@stephencelis I've also added cancellation to UIKitCaseStudies/NavigateAndLoad.swift. I believe LoadThenNavigate ones don't need this, right?

I think they do. If you back out of those studies while the request is in flight, they'll still write to state, which means if you drill back in it'll immediately double-drill you to the loaded state.

@filblue
Copy link
Contributor Author

filblue commented Sep 1, 2021

@stephencelis thank you for clarification, updated.

@filblue filblue requested a review from stephencelis September 1, 2021 19:40
stephencelis
stephencelis approved these changes Sep 1, 2021
View reviewed changes
Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

Thanks!

@stephencelis stephencelis merged commit dacd3de into pointfreeco:main Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephencelis stephencelis stephencelis approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@filblue @stephencelis