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

Version 1.1.0 #26

Merged
merged 18 commits into from
Aug 16, 2024
Merged

Version 1.1.0 #26

merged 18 commits into from
Aug 16, 2024

Conversation

ssestak
Copy link
Contributor

@ssestak ssestak commented Aug 7, 2024

  • Template Updates
    • Removed SwiftLint warnings
    • Omitted application prefix in naming
    • Dropped class prefix in mock view model events
  • Added commentary explaining anti-pattern of adding view modifiers in coordinator root view
  • Alerts no longer managed by coordinators; their presentation is now individual view's responsibility
  • Scene Delegate
    • Introduced view modifier in helpers
  • Destination IDs
    • Integrated @EnumIdentifiersGenerator macro for auto-generating code for Hashable and Identifiable protocol conformance
    • The macro produces nested enum for ID generation for enum cases
  • Introduced FlowProvider in helpers
  • For coordinators, integrated ModalCoverModel<Destination> to avoid UI confusion regarding sheet and full screen cover presentation order

Prerequisites for Merging:

  • New projects must specify a key-value pair for LaunchScreen in Info.plist to ensure proper display
  • Macro
    • Requires importing EnumIdentifiersGenerator currently, but ideally, it should be part of FuturedArchitecture
    • It doesn't account for situation when Destination ID needs to be computed using an associated value
      • UPDATE: DONE

Macro Expansion Example:
image

@ssestak ssestak mentioned this pull request Aug 7, 2024
@ssestak ssestak requested a review from samoilyk August 7, 2024 12:10
samoilyk
samoilyk previously approved these changes Aug 8, 2024
samoilyk
samoilyk previously approved these changes Aug 15, 2024
Copy link
Contributor

@mikolasstuchlik mikolasstuchlik left a comment

Choose a reason for hiding this comment

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

Overall a great piece of pioneering work!

samoilyk
samoilyk previously approved these changes Aug 16, 2024
- This end destination should trigger a scene display outside of the scene provider.
2. Other destinations will be used to display scenes defined within the scene provider.

# Example #
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Example #
# Example #
Declare a `protocol` conforming to the composition. The protocol should only be conformed to by enums. We use the requirements to force appearance of certain cases: [SE-0280](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0280-enum-cases-as-protocol-witnesses.md).

Comment on lines 21 to 22
static var destination: Self { get }
static var end: Self { get }
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should use naming which does not appear to have a special meaning, if it is only an example.

Suggested change
static var destination: Self { get }
static var end: Self { get }
static var someDestination: Self { get }
static var otherDestionation: Self { get }



/**
A protocol providing an interface for reusable scene flow providers.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a reusable scene flow provider? Is my suggestion correct?

Suggested change
A protocol providing an interface for reusable scene flow providers.
A protocol providing an interface for reusable scene flow providers. *Reusable scene flow provider* is part of a scene flow, which can be used as a part of more *Flow Coordinators*. The shared section of the flow is taken out of the *Flow Coordinator* and placed into a class conforming to `CoordinatorSceneFlowProvider`.

@@ -30,11 +30,8 @@ final class ___VARIABLE_flowCoordinatorIdentifier___FlowCoordinator: NavigationS
}

extension ___VARIABLE_flowCoordinatorIdentifier___FlowCoordinator {
enum Destination: String, Hashable, Identifiable {
@EnumIdentable
enum Destination: Hashable, Identifiable {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think we should make the @EnumIdentable add the conformance. I don't think it's MUST-HAVE at this point, but please, file an Issue to this effect.

Comment on lines 12 to 13


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 48 to 54
extension ___VARIABLE_sceneFlowProviderIdentifier___SceneFlowProvider {
@EnumIdentable
enum Destination: ___VARIABLE_sceneFlowProviderIdentifier___FlowDestination {
case destination
case end
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why we need to do this. The Destination should be only a generic parameter of the ___VARIABLE_sceneFlowProviderIdentifier___SceneFlowProvider and conformed to by the Destinations of the Flow cooridnators.

If I am not correct, why do we need ___VARIABLE_sceneFlowProviderIdentifier___FlowDestination at all?

@mikolasstuchlik
Copy link
Contributor

It is still not fully clear to me, how the Scene Provider should work. I don't want to hinder the acceptance because of the documentation. I have made some requests and the rest of it can be deferred to an Issue (on both FuturedKit and macros).

However, I'm reluctant to merge Scene Provider in this state.

@mikolasstuchlik
Copy link
Contributor

86580cded392542b51fe48041237db67

@ssestak ssestak merged commit 28288d2 into main Aug 16, 2024
1 check passed
@ssestak ssestak deleted the feature/v1.1.0 branch August 16, 2024 15:11
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.

3 participants