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 hiddenWhenSkeletonIsActive property #339

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Add hiddenWhenSkeletonIsActive property #339

merged 2 commits into from
Oct 19, 2020

Conversation

mohn93
Copy link
Contributor

@mohn93 mohn93 commented Oct 6, 2020

Summary

Add hiddenWhenSkeletonIsActive property which will hide the view when the skeleton animation starts

Requirements (place an x in each of the [ ])

@Juanpe
Copy link
Owner

Juanpe commented Oct 6, 2020

Hi @mohn93, thanks for your first contribution 👍

I would like to discuss about that.

Checking your approach...I think it could be a good solution...But my first approach was to notify when the skeleton will/did appear/disappear. Then the developers can modify the views as they want.

What do you think?

@mohn93
Copy link
Contributor Author

mohn93 commented Oct 6, 2020

Hi @Juanpe

How your approach will look like can you show me an example, please?

@Juanpe
Copy link
Owner

Juanpe commented Oct 6, 2020

Yep, there are two ways to implement it.

Way 1

// override this method when you need to do something before the skeleton appears
extension UIView {
 @objc func skeletonWillAppear() { }
}

Way 2

protocol SkeletonViewDelegate {
 func skeletonViewWillAppear(_ view: UIView)
}

Do the same for did appear and will/did disappear

@mohn93
Copy link
Contributor Author

mohn93 commented Oct 7, 2020

Cool
But you know sometimes I need to hide specific view without any boilerplate, so what if I needed to hide a punch of random views, in some hierarchy, and they are not custom views where it's not cool to add more boilerplate to the code, my approach would be more cleaner.

Since I needed this feature in more than one place, I think it would be good if it's just a simple property or anything near this approach.

And no harm if you kept the other two ways you laid out, the developer might need to do something unfamiliar or worth the boilerplate.

@Juanpe
Copy link
Owner

Juanpe commented Oct 7, 2020

mmm...actually, I think you're right, as an open-source project we should provide multiples ways to do something. So yep, we're going to include your approach, although we include later the rest of the approaches.

So, I'm going to review the code, because I have some doubts.

Also, I think we should include this new property in the README file, to be clear that you can hide views when the skeleton is active, ok?

Thanks

@mohn93
Copy link
Contributor Author

mohn93 commented Oct 7, 2020

Cool, yeah it needs reviewing, maybe you have some considerations that I didn't take in my implementation, so take your time

@Juanpe
Copy link
Owner

Juanpe commented Oct 15, 2020

Hi @mohn93, do you know approx when you can update the doc? Thanks

@mohn93
Copy link
Contributor Author

mohn93 commented Oct 16, 2020

Hi @mohn93, do you know approx when you can update the doc? Thanks

@Juanpe Sorry for the delay, I've just updated the documentation

@Juanpe
Copy link
Owner

Juanpe commented Oct 19, 2020

Thanks for your first contribution 🎉

@Juanpe Juanpe merged commit c329553 into Juanpe:develop Oct 19, 2020
@ahmedgomaa27
Copy link

@Juanpe can you release a new version for this change ?

@Juanpe
Copy link
Owner

Juanpe commented Oct 28, 2020

Hi @ahmedgomaa27, Next version will be released today :)

@Juanpe
Copy link
Owner

Juanpe commented Oct 29, 2020

version 1.11.0 was published 👍

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