Shopify / FunctionalTableData

Declarative UITableViewDataSource implementation
MIT License
365 stars 30 forks source link

Added cell selection state to cell actions. #201

Closed antonino-u closed 3 years ago

antonino-u commented 3 years ago

UITableViewCell has a selected property, which is automatically toggled through the UITableView in many cases. For example, if a user taps on a cell it is first highlighted then selected. If a user swipes on a cell, then the table view determines that that cell is now selected and shows the leading or trailing swipe actions accordingly.

If the table view supports multi-selection, then the table view shows checkboxes as a left accessory view of the cell, and if a cell is selected (via either a double-finger swipe gesture or a single tap), then its index path is added to indexPathsForSelectedRows automatically.

If, however, the tableView is reloaded, or simply displaying for the first time and we want certain cells pre-selected, then indexPathsForSelectedRows is reset to nil. Therefore, it isn't a reliable way of keeping track of which rows are selected. selected: Bool? in CellStyle fixes this and now whenever the table view is rendered or its data is reloaded (pull-to-refresh, pagination, etc), the selections are not lost.

In the same vein, just like how the table view needs to be in editing mode for multi-select to work, the function public func tableView(_ tableView: UITableView, canEditRowAt indexPath: IndexPath) -> Bool is called on UITableViewDataSource to confirm that an individual cell can have a checkbox in multi-select mode. Therefore, selected is checked in the internal hasEditActions computed property to make sure that the right cells can be selected and deselected.

raulriera commented 3 years ago

Unsure if CellActions should be handling the SelectionState, but there are 2 legitimate things I think this PR should address:

antonino-u commented 3 years ago

Unsure if CellActions should be handling the SelectionState, but there are 2 legitimate things I think this PR should address:

  • UICollectionView support
  • Tests

Hey @raulriera ! Take another look. I added a test, and moved selection to CellStyle and it works nicely! Note: the test failure has nothing to do with my changes:

xcodebuild: error: Failed to build project FunctionalTableData with scheme FunctionalTableData-Package. 15 Reason: Cannot test target “FunctionalTableDataTests” on “iPhone 8”: iPhone 8’s iOS Simulator 13.4.1 doesn’t match FunctionalTableDataTests’s iOS Simulator 14.0 deployment target. 16

What should I do about this? Update github actions?

antonino-u commented 3 years ago

@raulriera and I had an offline conversation where we discussed the right place for the selection state, and I ended up agreeing that CellStyle was probably better. Also, I have created an issue to apply this to UICollectionView and in the documentation for the property I mentioned that it was only for UITableView. I also added tests for it. I will merge this because it's going to make my life way easier, and if anyone would like to request changes I'm happy to discuss further :)

raulriera commented 3 years ago

Thanks or the changes