Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Refactor ELCAssetTablePicker to be a UICollectionViewController #664

Conversation

J-Swift
Copy link
Contributor

@J-Swift J-Swift commented Feb 26, 2019

Please take a moment to fill out the following:

Cleans up #650

Changes Proposed in this pull request:

ELCAssetTablePicker, likely due to its noted age in the original PR, was acting a lot like a poor mans UICollectionViewController. I've made it a less-poor mans implementation deriving from CollectionView and tried to clean the API up a bit. This should be easier to maintain and behave less quirky (i.e. not having to listen to rotation changes and manually recalculate layout).

I didn't do this here, but I also think we should unify the single-select vs multi-select codepaths, and not use two different view controllers in the two cases. I assume this wasn't done because the multi-select was just introduced and so is untested? I actually stumbled on this refactor because I was trying to debug a multi-select issue and wasn't aware I was looking at the wrong view controller : )

Also, a disclaimer that I am very new to Xamarin/C#, so please excuse any idiom violations.

@jamesmontemagno
Copy link
Owner

@vatsalyagoel can you look at this?

@vatsalyagoel
Copy link
Contributor

I'm currently travelling. Will have a look once I get back next week. :)

@J-Swift
Copy link
Contributor Author

J-Swift commented Apr 10, 2019

@vatsalyagoel - pinging here to see if you had a chance to review.

I'd love to get this in so I can look at unifying single/multi-select. I'm not a big fan of the custom modal on tablet, so I'm hoping to get some time to switch that to a 'proper' modal at some point.

@vatsalyagoel
Copy link
Contributor

Hi @J-Swift,

I'll go through the changes today. They look good just need to do some more testing :)

@vatsalyagoel
Copy link
Contributor

@jamesmontemagno, @J-Swift this looks good to go.

@jamesmontemagno jamesmontemagno merged commit 2ab437c into jamesmontemagno:master Apr 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants