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

[NT-145] Shipping rules search #834

Merged
merged 20 commits into from
Sep 13, 2019
Merged

[NT-145] Shipping rules search #834

merged 20 commits into from
Sep 13, 2019

Conversation

dusi
Copy link
Contributor

@dusi dusi commented Sep 10, 2019

📲 What

Adds the ability to search shipping rules picker.

🤔 Why

Shipping rules picker usually comes with a very long list of locations that we want our users to be able to filter down based on the search term.

🛠 How

The VM had to be completely re-written in order to support search & selection.

😱 Please note that this PR is marked as Big, because there's a lot of tests coverage. The tests should be very descriptive and include a lot of comments.

👀 See

Untitled

✅ Acceptance criteria

  • Selecting a shipping rule from the list (that has previously NOT been filtered) updates selected shipping rule properly and dismisses the picker
  • Selecting shipping rule from the list (that has been previously filtered/searched) updates selected shipping rule properly and dismisses the picker
  • Searching the list filters down the list to only the shipping rules that start with the searched query
  • Searching the list scrolls selected shipping rule to the top (if it's included in the filtered list)
  • Opening up the picker always scrolls to the initial shipping rule (no matter where in array it was (first, middle, last)
  • Clearing up the search input restores the original (unfiltered) list
  • Tapping the Cancel button dismisses the picker

.observeValues { [weak self] indexPath in
self?.tableView.cellForRow(at: indexPath)?.accessoryType = .none
.observeValues { [weak self] _ in
self?.tableView.visibleCells.forEach { $0.accessoryType = .none }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This simplified a lot of logic, because we don't have to keep in track previously selected index (this gets really complicated in case the list is filtered causing that previous index to be out of sync).

To simplify this behaviour we simply make sure to deselect cells first, then selecting the latest index.

.filter { oldIndex, newIndex in oldIndex != newIndex }
.map(second)
let searchText = self.searchTextDidChangeProperty.signal
.ksr_debounce(.milliseconds(100), on: AppEnvironment.current.scheduler)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a little bit of delay so that this functionality doesn't cause a bottle neck on slower devices when typing really fast.

@ksr-ci-bot
Copy link

ksr-ci-bot commented Sep 10, 2019

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@dusi dusi changed the title [NT-128] Shipping rules search [NT-145] Shipping rules search Sep 10, 2019
Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

Some initial comments/questions. Good job on this though it's a deceptively complex feature!

Library/ViewModels/ShippingRulesViewModel.swift Outdated Show resolved Hide resolved
Library/ViewModels/ShippingRulesViewModel.swift Outdated Show resolved Hide resolved
Library/ViewModels/ShippingRulesViewModel.swift Outdated Show resolved Hide resolved
Library/ViewModels/ShippingRulesViewModel.swift Outdated Show resolved Hide resolved
Library/ViewModels/ShippingRulesViewModel.swift Outdated Show resolved Hide resolved
@@ -9,14 +9,19 @@ public struct ShippingRuleData: Equatable {
public let shippingRule: ShippingRule
}

private typealias ShippingRulesInputData = (
project: Project, shippingRules: [ShippingRule], selectedShippingRule: ShippingRule
Copy link
Contributor

Choose a reason for hiding this comment

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

Might I suggest initialSelectedShippingRule instead of selectedShippingRule to differentiate between the shipping rule that the VC is initially configured with and any future selected rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I called this type Input...but I see how this still could be confusing. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically we're never really accessing this one from the tuple value ¯_(ツ)_/¯

Library/ViewModels/ShippingRulesViewModel.swift Outdated Show resolved Hide resolved
Library/ViewModels/ShippingRulesViewModel.swift Outdated Show resolved Hide resolved
@dusi dusi requested a review from ifbarrera September 13, 2019 00:13
Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

Works great! Nice job! :shipit:

@dusi dusi merged commit fee31be into master Sep 13, 2019
@dusi dusi removed the needs review label Sep 13, 2019
@dusi dusi deleted the shipping-rules-search branch October 4, 2019 20: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

3 participants