Navigation Menu

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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RefreshControlDesignable example #429

Merged
merged 9 commits into from Apr 4, 2017

Conversation

tbaranes
Copy link
Member

@tbaranes tbaranes commented Apr 2, 2017

Related to #238 and #425

A few things in this PR:

  • Make AnimatableTableView conforms to RefreshControlDesignable, iOS10 only, I think we can add a support for lower version, by adding manually the refreshControl. Is it worth it?
  • Add RefreshControlDesignable examples in playground:
    • For UITableViewController.
    • For UITableView

Seems big, but its mainly storyboards!

Just one thing: I'm getting these warnings in the console for the UITableViewController example, but don't know why 馃

Failed to set (hasRefreshControl) user defined inspected property on (IBAnimatableApp.RefreshControlTableViewController): Could not load NIB in bundle: 'NSBundle <../IBAnimatableApp.app> (loaded)' with name '' 
Failed to set (refreshControlTintColor) user defined inspected property on (IBAnimatableApp.RefreshControlTableViewController): Could not load NIB in bundle: 'NSBundle <../IBAnimatableApp.app> (loaded)' with name '' 
Failed to set (refreshControlBackgroundColor) user defined inspected property on (IBAnimatableApp.RefreshControlTableViewController): Could not load NIB in bundle: 'NSBundle <../IBAnimatableApp.app> (loaded)' with name '' 

@IBAnimatableBot
Copy link

IBAnimatableBot commented Apr 2, 2017

1 Warning
鈿狅笍 Big PR

Generated by 馃毇 Danger

@phimage
Copy link
Member

phimage commented Apr 2, 2017

For the warning I think it's because we play with the refreshControl before view loading
That's why I add in configureRefreshController

 guard !isViewLoaded else {
      return
    }

https://github.com/IBAnimatable/IBAnimatable/blob/master/IBAnimatable/RefreshControlerDesignable.swift

Adding for the previous ios version is welcome 馃憤
I want to work on that later, because I want to add support to collection too.

@tbaranes
Copy link
Member Author

tbaranes commented Apr 2, 2017

Wups, I miss this. You are right the warning has been fixed by putting back the guard, good catch! Thanks

@tbaranes
Copy link
Member Author

tbaranes commented Apr 2, 2017

Added support of AnimatableTableView RefreshControlDesignable for iOS version inferior to 10.

There's still a restriction: the user has to add himself the refresh control to its tableView, then call the configureRefreshControls (such as in the example). That make the usage harder, and ask for more code than the iOS10 version. Still, I choose this because I think it would have been even more complicated to ask the user to search its refreshControl in the tableView's subviews in order to add a target.

What do you prefer? I'm ok to change, and I will update the docs following the final decision

Copy link
Member

@JakeLin JakeLin left a comment

Choose a reason for hiding this comment

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

@tbaranes it is very good to support refresh control for pre-iOS 10. I have an idea: Can you add it automatically for pre-iOS 10? If we can't, I think we need to add the example code to AnimatableTableView as a comment.

Apart from that, all good 馃憤, well done 馃挭

CHANGELOG.md Outdated
@@ -16,8 +16,8 @@ All notable changes to this project will be documented in this file.
[#403](https://github.com/IBAnimatable/IBAnimatable/pull/403) by [@tbaranes](https://github.com/tbaranes)
- Make images of `AnimatableSlider` designable.
[#417](https://github.com/IBAnimatable/IBAnimatable/pull/417) by [@phimage](https://github.com/phimage)
- Add `AnimatableTableViewController` to support `UIRefreshControl` customization.
[#418](https://github.com/IBAnimatable/IBAnimatable/pull/418) by [@phimage](https://github.com/phimage)
- Add `RefreshControlDesignable` to make `UIRefreshControl` customization available in interface builder. Currently supported by `UITableViewController` and `UITableView`
Copy link
Member

Choose a reason for hiding this comment

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

Interface Builder 馃槄

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed


override public func viewDidLoad() {
super.viewDidLoad()

// Install action on refresh
self.refreshControl?.addTarget(self, action: #selector(RefreshTableViewController.refresh(_:)), for: .valueChanged)
self.refreshControl?.addTarget(self, action: #selector(refresh(_:)), for: .valueChanged)
Copy link
Member

Choose a reason for hiding this comment

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

Remove self here?

attributes[NSForegroundColorAttributeName] = color
}
refreshControl.attributedTitle = NSAttributedString(string: "\(Int(time))", attributes: attributes )

DispatchQueue.main.after(1) { [unowned self] in
self.updateMessage(refreshControl: refreshControl, time: time - 1)
DispatchQueue.main.after(1) { [weak self] in
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to use [weak self] or [unowned self] for DispatchQueue.main. They are static methods.

} else {
refreshControl = nil
var refreshControl = self.subviews.first { $0 is UIRefreshControl } as? UIRefreshControl
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this automatically? For example, we check there is a UIRefreshControl or not. Add one if not.

Copy link
Member Author

@tbaranes tbaranes Apr 3, 2017

Choose a reason for hiding this comment

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

We can, see my answer below!

} else {
refreshControl = UIRefreshControl()
tableView.addSubview(refreshControl!)
tableView.configureRefreshController()
Copy link
Member

Choose a reason for hiding this comment

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

I made a comment on https://github.com/IBAnimatable/IBAnimatable/pull/429/files#diff-f56d938957f5c1840f24beee6993086fR43

If we can't do it automatically, we may need to put this code to AnimatableTableView as a comment for pre-iOS 10.

@tbaranes
Copy link
Member Author

tbaranes commented Apr 3, 2017

@JakeLin We can add it automatically, but that means when the user will add a target, he will have to look into IBAnimatable to see how to get it then bind it. The example RefreshControlTableViewViewController (yeah, that's a big name 馃槂) is working for both case, iOS10 and pre-10. Just need to update the documentation following our final decision.

What do you prefer? Should I still add it automatically?

@JakeLin
Copy link
Member

JakeLin commented Apr 4, 2017

@tbaranes I think what you are doing is good. Please merge it when you think it is ready.

@tbaranes tbaranes force-pushed the feature/refresh_control_example branch from 781901d to 40424d9 Compare April 4, 2017 06:27
@tbaranes tbaranes merged commit d445b4a into master Apr 4, 2017
@tbaranes tbaranes deleted the feature/refresh_control_example branch April 4, 2017 06:31
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

4 participants