jamonholmgren / ProMotion

ProMotion is a RubyMotion gem that makes iPhone development less like Objective-C and more like Ruby.
MIT License
1.26k stars 148 forks source link

Pull to refresh in Table Screen #43

Closed Swatto closed 11 years ago

Swatto commented 11 years ago

It could be great to have a simple config to make a pull refresh enabled for a table view. Imagine if the data came from a json call or whatever, you can call the update_table_data.

We can use the UIRefreshControl for iOS 6 and use CKRefreshControl for older version : we can use the same code for iOS 5 and 6 with that.

It could be somethings like :

class SettingsScreen < ProMotion::TableScreen
  title "Settings"
  pullToRefresh true

  def on_load
    add_right_nav_button(label: "Save", action: :save)
    set_tab_bar_item(title: "Settings", icon: "settings.png")
  end

  def table_data
    ...
  end
end

What do you think about that ?

macfanatic commented 11 years ago

I like the idea, and have implemented in my own projects with a few lines of code before as well.

I would rather use snake cased method name and the ability to pass a block when the user pulls down on the control to begin the update itself.

class SettingsScreen < PM::TableScreen

  pull_to_refresh # no options just turns the feature on

  # have the block respond to the UIControlEventValueChanged event
  pull_to_refresh do |control|
    # interact with your API possibly?
  end

  # optionally, pass a symbol to call a method instead of a block
  pull_to_refresh :update_data_from_server

end
Swatto commented 11 years ago

I like your syntax of your proposition. I have a question : what's the value of the control variable in the pull_to_refresh do |control| ?

And what about the portability of that feature ? Do you think we can do that if the base deployment target of Promotion is 5.0 ?

macfanatic commented 11 years ago

control would be the same as self.refreshControl in iOS6, and possibly even in iOS5 from looking at the documentation from CKRrefreshControl, although I'm not sure. Either way, when observing an UIControlEventValueChanged event you can access the object that sent the event, and that's what I would pass along here as control.

Regarding iOS5 compatibility, I don't see why this wouldn't work - the implementation details would have to deal with that, but for the end PM user, I don't think they would ever know it was going on, as it should be.

Swatto commented 11 years ago

Ok thanks for the information for the UIControlEventValueChanged

Yeah I think you're right for the iOS 5.

It could be a very nice feature I think !

jamonholmgren commented 11 years ago

We implemented pull-to-refresh with the PHFRefreshControl pod in our latest app, but I agree that the iOS 6 version would be useful built-in to PM. CKRefreshControl says "100% API-compatible with UIRefreshControl in iOS 6" so that would work.

Would we automatically include the CKRefreshControl pod in ProMotion? I'm not sure that's possible yet with RubyMotion gems.

Swatto commented 11 years ago

I think so solution would be to have that kind of code in the base of PM (I find it in the Joybox project) :

unless defined?(Motion::Project::Config)
  raise "The ProMotion gem must be required within a RubyMotion project Rakefile."
end

Motion::Project::App.setup do |app|
  ...
  app.pods do
    pod 'CKRefreshControl'
  end
end
macfanatic commented 11 years ago

I just did this with my latest gem, although there is a bug that I filed with motion-cocoapods that requires the user to specify the pod in their rake file, as you can currently only successfully call "app.pods" once (contrary to all the docs).

jamonholmgren commented 11 years ago

Planned for 0.6.0.

macfanatic commented 11 years ago

Here's the before mentioned bug in motion-cocoapods where you can't call app.pods more than once in an entire project.

markchang commented 11 years ago

Is there a way to to do this with the current ProMotion::TableScreen ?

markrickert commented 11 years ago

Yes. See my implementation here: https://github.com/Skookum/RubyMotionTalk/blob/master/RedditRss/app/Screens/RedditRssViewController.rb

markchang commented 11 years ago

Hah @markrickert! I somehow found that right after I posted, copied the implementation, and it works great. Thanks.

markrickert commented 11 years ago

Yeah, I posted it to the RubyMotion google group earlier this evening as an example of what you can do with RubyMotion. Frankly I'm pretty impressed with myself that I wrote a reddit RSS feed reader in like 50 lines of code, lol

markchang commented 11 years ago

Ah! That's where I found it. Thanks again.

markrickert commented 11 years ago

:+1: No worries. Glad to help.

jamonholmgren commented 11 years ago

Your implementation looks pretty nice, Mark!

markrickert commented 11 years ago

Credit goes to @tkadauke for the implementation in his TinyMon client - https://github.com/tkadauke/TinyMon/blob/e12bfef41dae802498fba6f268108d2405a414bf/lib/mixins/refreshable.rb

I just implemented it inline with my view controller. Thanks, Thomas! ...you open sourcing your app was a great help to me!

markrickert commented 11 years ago

I'm going to try and work on implementing this. I think that it would work just like the remote images in that if you're on ios <6, you'd have to include the pod in your main Rakefile otherwise it simply wont work (just like the SDWebImage dependency)

jamonholmgren commented 11 years ago

:+1:

markrickert commented 11 years ago

Hey everyone, check out my initial work for a refreshable tableview here: https://github.com/markrickert/ProMotion/tree/version-0.6-table-refreshable

I also added some documentation on how to use it: https://github.com/markrickert/ProMotion/tree/version-0.6-table-refreshable#tablescreen

Let me know you're initial thoughts.

TODO:

  1. Make it work on iOS < 6.0
  2. Better block handling?
jamonholmgren commented 11 years ago

Looks really good! I like the refreshable name.

I do have some feedback on this. I think we should be able to specify the method to call in the refreshable specification.

class IndexScreen < PM::TableScreen
  refreshable :pull_data_from_server

  def pull_data_from_server
    some_async_call do
      end_refreshing
      update_table_data # Should this be called implicitly by `end_refreshing`?
    end
  end  

  # ...
end

If you don't pass in a method name, call on_refresh by default. So your method definition could be:

  def refreshable(callback = :on_refresh)
    # ...
  end
jamonholmgren commented 11 years ago

For < iOS 6, we can just let devs know to include the CKRefreshControl CocoaPod and it'll work.

markrickert commented 11 years ago

Cool. i'll update my code some this evening. Then I have to write tests for it :wink:

jamonholmgren commented 11 years ago

A sample screen, a little more fleshed out:

class IndexScreen < PM::TableScreen
  searchable placeholder: "Search items"
  refreshable

  def on_refresh
    Item.pull_from_server_async do |items|
      end_refreshing
      @items = items
      update_table_data
    end
  end

  def on_load
    start_refreshing
  end

  def table_data
    [{ cells: @items.map { |item| cell_for_item(item) } }]
  end
end

Something like that.

markrickert commented 11 years ago

OK, I think it's about done from an implementation standpoint other than ios5 support.

https://github.com/markrickert/ProMotion/commit/5a57ab6005d15d77b5e81ec4943719556a381e6b

you can just cal refreshable or you can do the balls-to-the-wall customization like this:

refreshable(
  callback: :on_refresh,
  pull_message: "Pull to refresh", 
  refreshing: "Refreshing data…", 
  updated_format: "Last updated at %s", 
  updated_time_format: "%l:%M %p"
)

As you can see, even the text and time format displayed are customizable :)

screen shot 2013-05-06 at 7 12 06 pm

markrickert commented 11 years ago

Also just implemented a warning if the user doesn't define the callback method in their class. https://github.com/markrickert/ProMotion/commit/45d726133cb6e9527d587b383b4f0eac4cd670b9

Last thing to go: ios5 support.

markrickert commented 11 years ago

OK, i believe this thing is feature complete... just gotta write some tests and a pull request will be coming your way!

markrickert commented 11 years ago

Anyone want to help on the tests? I'm stuck and for some reason can't get the table screen data to load into the spec, so when I try and get views or tap cells, it doesn't work :(

https://github.com/markrickert/ProMotion/blob/version-0.6-table-refreshable/spec/table_screen_spec.rb

When i run rake spec, the simulator doesn't show the cells in the tableview. Wondering if someone with more testing experience might be able to figure it out

jamonholmgren commented 11 years ago

I'll help -- I'm working on ProMotion right now and your PR is one of the last things before releasing 0.6. Can you just submit your PR right now? Do it to the 0.6 branch.

jamonholmgren commented 11 years ago

Oh, can you include a (non "sonofabitch") version of your screenshot in the README? I really want to include more screenshots for features.

markrickert commented 11 years ago

haha... that was just an example screen shot for the discussion thread. I'm not planning on putting a screenshot of the pull to refresh in the readme :)

markrickert commented 11 years ago

PR submitted. Thanks for the help & guidance on this! I'm glad to have made an impact on 0.6!

jamonholmgren commented 11 years ago

Yeah, I know :D -- but I really want more screenshots in the README at some point. I'm not sure people realize everything ProMotion can do.

markrickert commented 11 years ago

Ahh... I misrread the comentt.

Yeah, i can use the default table stuff and include it with a ss of the refresh controller.

markrickert commented 11 years ago

Here's a few screenshots:

screeny video may 7 2013 12 34 48 pm 04

screeny video may 7 2013 12 34 48 pm 15

And here's a super fancy gif for you ;)

pull-to-refresh

jamonholmgren commented 11 years ago

Nice! I'll pull it into the README before launching 0.6.

markrickert commented 11 years ago

It surprisingly hard to get a screenshot of the pull to refresh menu in a transitioning state... I wound up having to record a video and then extract the image, lol.