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

Added a new property for single selection image size. #19

Merged
merged 3 commits into from
Sep 23, 2016

Conversation

LawrenceHan
Copy link
Contributor

For the reason of reducing memory usage, I strongly recommend to add this property for resizing single selection image. The current project only returns an origin image which is not enough.

@yahoocla
Copy link

yahoocla commented Sep 6, 2016

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@yahoocla
Copy link

yahoocla commented Sep 6, 2016

CLA is valid!

@LawrenceHan
Copy link
Contributor Author

LawrenceHan commented Sep 6, 2016

@stewwu @ZZBHuang Please review.

/**
* @brief Use this property to customize the returned item type for single selection. Default value is NO.
*/
@property (nonatomic, assign) BOOL shouldReturnAssetForSingleSelection;
Copy link
Contributor

@yzlai yzlai Sep 22, 2016

Choose a reason for hiding this comment

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

After discussing with @stewwu, we'd agree return Asset is a better behavior.
However, we also want to make this change backward compatible. Thus, how about renaming this new property to shouldReturnImageForSingleSelection with default YES ?

@@ -213,6 +214,12 @@ - (void)collectionView:(UICollectionView *)collectionView didSelectItemAtIndexPa
if (indexPath.row == 0) {
[self yms_presentCameraCaptureViewWithDelegate:self];
}
else if (self.shouldReturnAssetForSingleSelection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about moving this condition checking under line 223 ?
This would prevent user enable multipleSelection and return Asset/Image at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yzlai Please review my latest commit.

Moved condition check under (self.allowsMultipleSelection) due to yzlai's comment.
Deleted demo test code.
@stewwu stewwu merged commit c61d2b8 into YahooArchive:master Sep 23, 2016
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.

4 participants