Improve canSelectAction, selectionAction#85
Improve canSelectAction, selectionAction#85raulriera merged 4 commits intoShopify:masterfrom dcramps:add-deselection-callback
Conversation
raulriera
left a comment
raulriera
left a comment
There was a problem hiding this comment.
Thanks for your contribution! glad to hear you are enjoying an using the library.
I like the changes and top hatting them seems to work fine. My concern is that this introduces a breaking change for everything using this library at the moment, since SelectionAction is so widely used. Would it be better to introduce a DeselectionAction instead? Having a breaking change and the same callback being called twice feels a bit weird.
|
|
||
| collectionView?.backgroundColor = .white | ||
| functionalData.collectionView = collectionView | ||
| collectionView?.allowsMultipleSelection = false |
There was a problem hiding this comment.
We don't need this here or in the tableview. This is the default value
| actions: CellActions( | ||
| canSelectAction: { callback in | ||
| callback(true) | ||
| }, |
There was a problem hiding this comment.
No need for the canSelectAction either 🙂 on any
There was a problem hiding this comment.
Ah yeah, leftovers from testing. I'll clean this up
That is an oversight 🙂 thanks for catching it |
| style: CellStyle(backgroundColor: .lightGray), | ||
| actions: CellActions( | ||
| canSelectAction: { callback in | ||
| callback(true) |
There was a problem hiding this comment.
It seems the value of this action is ignored in FunctionalCollectionData, using false will won't prevent selection
|
Updated to add the |
|
Thank you, although the |
|
Yeah, I figured I would leave that for a separate PR. TableData is doing it in |
raulriera
left a comment
raulriera
left a comment
There was a problem hiding this comment.
Thanks again for the changes! 👏
|
Feel free to merge it 🙂 |
|
@raulriera I think you'll have to merge, I don't have access (or something is hiding the merge button on GitHub 🤔) |
|
Oh sorry about that 🙂 i thought it wa available once it’s approved |
2 participants
Been using this framework in a project for a bit and so far it's been great, but a few things are missing that I would like to fix. Examples of changes are included in the demo app.
canSelectAction:
If this closure exists,
allowsMultipleSelectionis broken.willSelectRowAtnow checks forallowsMultipleSelectionand doesn't deselect the currently selected row if it's enabled.CollectionDatais also fixed forallowsMultipleSelection, although it does not currently callcanSelectActionat all. Not sure if that's intended or an oversight?selectionAction:
The current behaviour only calls this on
didSelect. It's useful to know when the user deselects as well, so the closure now sends a boolean indicating whether the user is selecting or deselecting.