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

#145 fixed issue where searching in inspector floret would not instan… #161

Merged

Conversation

Shukuyen
Copy link
Collaborator

@Shukuyen Shukuyen commented Mar 2, 2019

…tly load additional records

@Shukuyen Shukuyen requested a review from brototyp March 2, 2019 17:10
@brototyp-bot
Copy link

brototyp-bot commented Mar 2, 2019

3 Warnings
⚠️ Are there any changes that should be explained in the README.md?
⚠️ Cauli/Florets/Inspector/Record List/InspectorTableViewController.swift#L91 - Lines should not have trailing whitespace.
⚠️ Cauli/Florets/Inspector/Record List/InspectorTableViewController.swift#L102 - Lines should not have trailing whitespace.

Generated by 🚫 Danger

@Shukuyen Shukuyen requested a review from pstued March 2, 2019 18:46
@@ -111,6 +114,7 @@ extension InspectorTableViewController: UISearchResultsUpdating {
let searchString = searchController.searchBar.text ?? ""
dataSource.filterString = searchString
tableView.reloadData()
loadAdditionalRecords()
Copy link
Member

Choose a reason for hiding this comment

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

This would now load more data even if there is enough data visible. For example if one is typing "https://" and that will be part of every single request, this logic will load 8 more pages of records even though no more additional records would be needed.

Admittedly, this is pretty nitpicky. I'll think about if I have a better idea than that. If non of us has, I would be fine to merge it as it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Two ways to fix that:

  1. Calculate how many rows fit on the screen and only load that many rows.

  2. Move filtering of the record list to the storage, so that we can always load a new, filtered result set instead of filtering the current results.

I like 2 better, but I am not sure if it’s not too much for this issue.

Copy link
Member

Choose a reason for hiding this comment

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

I think you are correct. We should rather change how the Storage works and that's not part of this scope. There are quite some more tasks to tackle in that context.

Copy link
Member

Choose a reason for hiding this comment

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

I think for now I would love to change the loadAdditionalRecords() to a loadAdditionalRecordsIfNeeded() with the following implementation. What do you think about that?

    fileprivate func loadAdditionalRecordsIfNeeded() {
        let distanceToBottom = tableView.contentSize.height - tableView.frame.height - scrollView.contentOffset.y
        guard !scrolledToEnd, !isLoading, distanceToBottom < 100 else { return }
        isLoading = true
        let newRecords = cauli.storage.records(InspectorTableViewController.recordPageSize, after: dataSource.items.last)
        guard !newRecords.isEmpty  else {
            isLoading = false
            scrolledToEnd = true
            return
        }
        
        dataSource.append(records: newRecords, to: tableView) { [weak self] _ in
            self?.isLoading = false
        }
    }

…ew records if the scrollview is at the bottom
@brototyp brototyp merged commit 2b0557d into develop Mar 3, 2019
@brototyp brototyp deleted the bugfix/145-load-additional-matching-items-after-search branch March 3, 2019 16:28
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.

4 participants