search

praekeltfoundation / vumi-jssandbox-toolkit

Vumi JavaScript Sandbox toolkit.
BSD 3-Clause "New" or "Revised" License
7 stars 8 forks source link

Less naive splitting for PaginatedChoiceState #208

Closed rudigiesler closed 9 years ago

gsvr commented 9 years ago

@justinvdm @hodgestar Please review. Note I merged in #219 as I was expecting tests to change, so wait until that's merged, maybe?

This is probably not the most eloquent solution, but I didn't want to change too much of the existing code. Also, I did this over the weekend as a learning exercise, so no worries if you don't like it :)

hodgestar commented 9 years ago

@gsvr I've merged #219. Can you merge develop into this branch?

gsvr commented 9 years ago

@justinvdm @hodgestar I've addressed your comments (incl merging develop) if you want to have another look.

hodgestar commented 9 years ago

@gsvr I'm +1 on this landing after we have a comment explaining the discussion you and @justinvdm had (because I was wondering the same thing). Would like a +1 from @justinvdm too though.

justinvdm commented 9 years ago

I made really minor comment and style fixes, but otherwise looks great! I'm going to go ahead and land once Travis is happy.

justinvdm commented 9 years ago

Travis builds are taking a while to start, I suspect Travis is quite busy atm, will land in the morning once Travis happy.

hodgestar commented 9 years ago

:+1:

@justinvdm Tx for doing the style and comment fixes.