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

Add extension to has image property components #2174

Closed
wants to merge 11 commits into from

Conversation

Mx-Iris
Copy link
Contributor

@Mx-Iris Mx-Iris commented Dec 5, 2023

I found that there are a lot of objects with image property, if I add setImage method for them one by one will lead to a lot of repetitive code, so I abstracted a protocol, in the future as long as the object with the image property to conform with this protocol can get setImage method.

@Mx-Iris Mx-Iris changed the title Add extension to has image property components AppKit: Add extension to has image property components Dec 5, 2023
@Mx-Iris Mx-Iris changed the title AppKit: Add extension to has image property components Add extension to has image property components Dec 5, 2023
@onevcat
Copy link
Owner

onevcat commented Apr 28, 2024

@Mx-Iris

Thank you for your patience and for submitting your pull request to the Kingfisher library. I apologize for the delayed response, as I only had the opportunity to review it thoroughly today.

I am impressed with the functionality introduced in your PR, and I see great value in integrating it into the Kingfisher library. However, we have recently made several updates to improve strict concurrency, including adding @MainActor annotations to image setting methods. Given these changes, I believe your contribution would fit perfectly in the upcoming major release.

Could you please rebase your changes onto the latest v8 branch and submit a new PR? I am eager to review it and include it as a stable feature in our next release.

Thank you once again for your valuable contribution and effort.

@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented Apr 30, 2024

I have merged the latest code and added some UIKit components, I found that there are a lot of UIConfigurations that have the image property, but this implementation of mine only takes into account AnyObject, and if I need to be compatible with the value type, I need to change the base property of KingfisherWrapper from let to var, do you have a Do you have a better way to implement this feature?

@onevcat
Copy link
Owner

onevcat commented May 3, 2024

Don't worry! Let me try to handle it.

@onevcat onevcat changed the base branch from master to v8 May 3, 2024 08:50
@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented May 3, 2024

Ok, thanks for submitting a new PR for me and adding concurrency, I'm not familiar with swift concurrency so I didn't understand what you said above, I'll close this PR then, feel free to @ me if you have any questions!

@Mx-Iris Mx-Iris closed this May 3, 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