OpenLP

Merge lp://qastaging/~dude-from-by/openlp/my-ios-remote into lp://qastaging/~danielborges93/openlp/ios-remote

  1. my-ios-remote
  2. Merge into ios-remote
Proposed by andrei
Status: Merged
Approved by: Daniel Borges
Approved revision: no longer in the source branch.
Merged at revision: 63
Proposed branch: lp://qastaging/~dude-from-by/openlp/my-ios-remote
Merge into: lp://qastaging/~danielborges93/openlp/ios-remote
Diff against target: 1480 lines (+657/-411)
15 files modified
OLP RemoteTests/PollAPITests.swift (+0/-128)
OLP RemoteTests/SearchViewModelTests.swift (+134/-0)
Podfile (+2/-0)
Podfile.lock (+10/-1)
Remote.xcodeproj/project.pbxproj (+24/-8)
Remote/AppDelegate.swift (+8/-14)
Remote/Classes/Controller/Search/SearchResultViewController.swift (+56/-51)
Remote/Classes/Controller/Search/SearchResultsViewModel.swift (+58/-0)
Remote/Classes/Controller/Search/SearchViewController.swift (+45/-39)
Remote/Classes/Controller/Search/SearchViewModel.swift (+62/-0)
Remote/Classes/Network/AddAPI.swift (+33/-21)
Remote/Classes/Network/LiveAPI.swift (+30/-21)
Remote/Classes/Network/PollAPI.swift (+23/-102)
Remote/Classes/Network/SearchAPI.swift (+32/-22)
Remote/Classes/Network/Service.swift (+140/-4)
To merge this branch: bzr merge lp://qastaging/~dude-from-by/openlp/my-ios-remote
Reviewer Review Type Date Requested Status
Daniel Borges Approve
Review via email: mp+322017@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2017-03-30.

Description of the change

Added ReactiveCocoa to the project, latest version and moved several services and search screen to it.
The main purpose of this changes is to ease testing. ReactiveCocoa provides bindings so we can rewrite project with MVVM paradigm and add tests to model and viewModel, where the main business logics lives. I've added SearchViewModel as an illustration. I am new to the swift version of this framework, so code might be clumsy. Added tests to searchviewmodel. Moved SearchResultsViewController to MVVM.

To post a comment you must log in.
Revision history for this message
Daniel Borges (danielborges93) wrote :

Hi Andrei. Sorry for delay.
Follows some things that can be fixed:

- The search is not working on iOS 8;
- Keep te search button aways enabled. Search with empty textfield will do a full search.

review: Needs Fixing
Revision history for this message
andrei (dude-from-by) wrote :

> Hi Andrei. Sorry for delay.
> Follows some things that can be fixed:
>
> - The search is not working on iOS 8;
> - Keep te search button aways enabled. Search with empty textfield will do a
> full search.

- fixed for ios8
- made search button enabled.

I don't like an option to keep it enabled - it is counter-intuitive. I've looked through many apps, most Apple apps I use - none of them uses such solution. Some of them display full list initially and non of them enables search button if there is nothing in the input field. As for now I will rollback to your solution, but in the future we may come up with one that is easier to understand for user.

Revision history for this message
andrei (dude-from-by) wrote :

> Hi Andrei. Sorry for delay.
> Follows some things that can be fixed:
>
> - The search is not working on iOS 8;
> - Keep te search button aways enabled. Search with empty textfield will do a
> full search.

- fixed for ios8
- made search button enabled.

I don't like an option to keep it enabled - it is counter-intuitive. I've looked through many apps, most Apple apps I use - none of them uses such solution. Some of them display full list initially and non of them enables search button if there is nothing in the input field. As for now I will rollback to your solution, but in the future we may come up with one that is easier to understand for user.

Revision history for this message
Daniel Borges (danielborges93) wrote :

I agree with you, but I prefer keep it working in this way before to enhance the usability of the search screen.

review: Approve
lp://qastaging/~dude-from-by/openlp/my-ios-remote updated
63. By andrei

Added ReactiveCocoa to the project; Added tests to searchviewmodel; Moved SearchResultsViewController to MVVM.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: