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

NavigationLinks are now supported in SwiftUI! #116

Merged
merged 40 commits into from Aug 31, 2021
Merged

Conversation

Tyler-Keith-Thompson
Copy link
Collaborator

@Tyler-Keith-Thompson Tyler-Keith-Thompson commented Aug 29, 2021

Linked Issue:

Checklist:


If Applicable:

  • Did you test when the first item is skipped?
  • Did you test when the last item is skipped?
  • Did you test when middle items are skipped?
  • Did you test proceeding backwards?

@Tyler-Keith-Thompson
Copy link
Collaborator Author

Tyler-Keith-Thompson commented Aug 29, 2021

As discussed I am deliberately working on just writing failing tests, that leaves the team to focus on production code. As of the time of writing there are more tests that need to be written but I am quite confident in the testing suite, I am also confident in our ability to get nav stacks working along with skipping one or more items, backing up, persistWhenSkipped persistence settings, and adding embedInNavigationView() to the API surface.

I suspect removedAfterProceeding will never animate the way we want it to, although I could be wrong. In UIKit this clearly means the next item slides in and the one you were just on gets removed from the back stack. While I am not at all confident in our ability to animate that the same way I do feel there's some middle ground where we just do it, and let SwiftUI animate that however it thinks is right. If that behavior is odd, or unlikely to be used that's fine, we're responding to a request from the consumer to remove the item after proceeding, and that is what we do.

NOTE: removedAfterProceeding may just outright work differently (see: very likely will) between Xcode 12.4, 12.5 and 13. SwiftUI changed a fair amount between all of those versions.

Alternative approaches for removedAfterProceeding:

  • Don't support it by doing some complex nonsense to prevent that as a possibility at compile time (I hate this for maintenance reasons, and because I do not think we will get the compiler error messages we'd want to make this nice)
  • Make setting the persistence a throwing function. On the surface this seems nice but in practice I suspect it will just make the consumer experience poor when trying to create views. Especially given that most of the time it won't fail.
  • Create a runtime crash if you have a presentationType of navigationLink and a persistence of removedAfterProceeding. This is sub-optimal because it's a runtime issue. That said it's not an option I'm substantially opposed to. I had originally thought that data driven workflows might make this hard-to-spot but data driven workflows will already have linting, so we could dis-allow the data model setting that presentation type and that persistence.

… was not able to verify that test is fully accurate - TT
Co-authored-by: Morgan Zellers <morgan.zellers@gmail.com>
Co-authored-by: Tyler Thompson <tyler.thompson@wwt.com>
Co-authored-by: Morgan Zellers <morgan.zellers@gmail.com>
Co-authored-by: Tyler Thompson <tyler.thompson@wwt.com>
Co-authored-by: Tyler Thompson <tyler.thompson@wwt.com>
Co-authored-by: Morgan Zellers <morgan.zellers@gmail.com>
…unction - tt mz

Co-authored-by: Tyler Thompson <tyler.thompson@wwt.com>
Co-authored-by: Morgan Zellers <morgan.zellers@gmail.com>
…ct things - tt mz

Co-authored-by: Tyler Thompson <tyler.thompson@wwt.com>
Co-authored-by: Morgan Zellers <morgan.zellers@gmail.com>
@Richard-Gist Richard-Gist added the enhancement New feature or request label Aug 30, 2021
@Richard-Gist Richard-Gist added this to the SwiftUI Support milestone Aug 30, 2021
@Richard-Gist Richard-Gist linked an issue Aug 30, 2021 that may be closed by this pull request
…ns of Xcode we support! ... we also broke testing and cannot fix it - TT RAG MZ

Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
Co-authored-by: Morgan Zellers <Morgan.Zellers@gmail.com>
Co-authored-by: Morgan Zellers <morgan.zellers@gmail.com>
Co-authored-by: Tyler Thompson <tyler.thompson@wwt.com>
…ing on that - TT RAG MZ

Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
Co-authored-by: Morgan Zellers <Morgan.Zellers@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2021

Codecov Report

Merging #116 (f209260) into main (568758f) will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
+ Coverage   93.98%   94.26%   +0.28%     
==========================================
  Files          75       76       +1     
  Lines        1813     1867      +54     
==========================================
+ Hits         1704     1760      +56     
+ Misses        109      107       -2     
Impacted Files Coverage Δ
...ftUI/Extensions/AnyWorkflowElementExtensions.swift 100.00% <100.00%> (ø)
...wiftCurrent_SwiftUI/Models/WorkflowViewModel.swift 100.00% <100.00%> (ø)
...SwiftCurrent_SwiftUI/Views/NavigationWrapper.swift 100.00% <100.00%> (ø)
...rces/SwiftCurrent_SwiftUI/Views/WorkflowItem.swift 94.44% <100.00%> (+2.77%) ⬆️
.../SwiftCurrent_SwiftUI/Views/WorkflowLauncher.swift 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0adcfba...f209260. Read the comment docs.

morganzellers and others added 9 commits August 30, 2021 14:11
… - MZ RAG

Co-authored-by: Morgan Zellers <morgan.zellers@gmail.com>
…s - MZ RAG

Co-authored-by: Morgan Zellers <morgan.zellers@gmail.com>
…he same type - MZ RAG

Co-authored-by: Morgan Zellers <morgan.zellers@gmail.com>
…rd broke, bringing it back though, also lets get compiling again for pure macOS - TT
…ier. There was a change needed to really support removedAfterProceeding behaving correctly - TT
morganzellers
morganzellers previously approved these changes Aug 31, 2021
…TT MZ

Co-authored-by: Morgan Zellers <Morgan.Zellers@gmail.com>
…igationView - TT MZ

Co-authored-by: Morgan Zellers <Morgan.Zellers@gmail.com>
…orrect locations in Jazzy now - TT MZ

Co-authored-by: Morgan Zellers <Morgan.Zellers@gmail.com>
@Tyler-Keith-Thompson Tyler-Keith-Thompson marked this pull request as ready for review August 31, 2021 15:32
@Tyler-Keith-Thompson Tyler-Keith-Thompson requested a review from a team as a code owner August 31, 2021 15:32
.github/guides/Getting Started with SwiftUI.md Outdated Show resolved Hide resolved
.github/guides/Getting Started with SwiftUI.md Outdated Show resolved Hide resolved
.github/guides/Getting Started with SwiftUI.md Outdated Show resolved Hide resolved
.github/abstract/How to use SwiftCurrent with SwiftUI.md Outdated Show resolved Hide resolved
.github/wiki/Getting-Started-Storyboard.md Show resolved Hide resolved
…tUI - TT rag

Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
…se SwiftCurrent with SwiftUI - RAG tt

Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
@Richard-Gist Richard-Gist added the documentation Improvements or additions to documentation label Aug 31, 2021
Copy link
Collaborator

@Richard-Gist Richard-Gist left a comment

Choose a reason for hiding this comment

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

I'm going to follow this up with a bigger documentation PR, but for an initial BETA release, this is fine.

@Richard-Gist Richard-Gist changed the title Navigation links NavigationLinks are now supported in SwiftUI! Aug 31, 2021
@Richard-Gist Richard-Gist merged commit c42b2aa into main Aug 31, 2021
@Richard-Gist Richard-Gist deleted the navigation-links branch August 31, 2021 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SwiftUI NavigationLink
4 participants