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/custom transitions #11

Merged
merged 17 commits into from
May 22, 2023
Merged

Feature/custom transitions #11

merged 17 commits into from
May 22, 2023

Conversation

erikdrobne
Copy link
Owner

@erikdrobne erikdrobne commented May 15, 2023

I've added support (API) for custom transitions.

@erikdrobne erikdrobne requested a review from tadelv May 15, 2023 15:11
Copy link
Collaborator

@tadelv tadelv left a comment

Choose a reason for hiding this comment

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

Nice! I like the idea of custom animations.

Sources/SwiftUICoordinator/Coordinator/Coordinator.swift Outdated Show resolved Hide resolved
Sources/SwiftUICoordinator/Navigator/Navigator.swift Outdated Show resolved Hide resolved
Sources/SwiftUICoordinator/Routing/RouterViewFactory.swift Outdated Show resolved Hide resolved
Sources/SwiftUICoordinator/Transition/Transition.swift Outdated Show resolved Hide resolved
Sources/SwiftUICoordinator/Utilities.swift Outdated Show resolved Hide resolved
@erikdrobne erikdrobne requested a review from tadelv May 16, 2023 13:13
view.navigationTitle(value)
}

.navigationTitle(route.title ?? "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I ran some tests and with this change, if the NavigationRoute.title property is nil, an empty string will be set as the title of the corresponding ViewController. This means that when pushing another controller on the stack, the back button will have an empty string instead of the system Back text.
Not sure if this is intentional or not?
Screenshot 2023-05-19 at 7 08 58 AM

Copy link
Owner Author

Choose a reason for hiding this comment

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

You're right, needs to be resolved before merging.

@erikdrobne erikdrobne merged commit d092ea5 into main May 22, 2023
1 check passed
@erikdrobne erikdrobne deleted the feature/custom-transitions branch May 22, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants