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

Update OC Compatiblity #2416

Closed
wants to merge 3 commits into from
Closed

Update OC Compatiblity #2416

wants to merge 3 commits into from

Conversation

mlch911
Copy link

@mlch911 mlch911 commented May 30, 2024

No description provided.

var animation: LottieAnimation? {
LottieAnimation.named(name, bundle: bundle, subdirectory: subdirectory)
}
private(set) lazy var animation = LottieAnimation.named(name, bundle: bundle, subdirectory: subdirectory)
Copy link
Member

Choose a reason for hiding this comment

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

Instead, what if we just make this:

Suggested change
private(set) lazy var animation = LottieAnimation.named(name, bundle: bundle, subdirectory: subdirectory)
var animation: LottieAnimation

and update the existing init to just:

  public init(
    name: String,
    subdirectory: String? = nil,
    bundle: Bundle = Bundle.main)
  {
    animation = LottieAnimation.named(name, bundle: bundle, subdirectory: subdirectory)
    super.init()
  }

Copy link
Author

Choose a reason for hiding this comment

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

No problem

@calda
Copy link
Member

calda commented May 31, 2024

Looks like the build is failing -- also make sure to run bundle exec rake format:swift to satisfy the linter. (Lottie follows Airbnb's Swift Style Guide).

@@ -46,6 +47,12 @@ public final class CompatibleAnimation: NSObject {
private let bundle: Bundle
}

extension LottieAnimation {
public func ocCompatible() -> CompatibleAnimation {
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? I'd prefer to keep the change focused on the new init

@calda
Copy link
Member

calda commented Jun 3, 2024

Still seeing build failures

@calda
Copy link
Member

calda commented Jun 14, 2024

Since the build is failing I'm going to go ahead and close this PR for now. If you'd like, feel free to re-open the PR after fixing the build errors!

@calda calda closed this Jun 14, 2024
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.

2 participants