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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintain arranged subviews array #56

Closed
wants to merge 4 commits into from
Closed

Maintain arranged subviews array #56

wants to merge 4 commits into from

Conversation

assafgelber
Copy link
Contributor

Hi!

Firstly, thanks for working on this, I just started using and it made my life a lot simpler! 馃樅
I was missing the arrangedSubview functionality, as also mentioned in #43, so tried to add it, including accompanying specs.

The solution is just for arrangedSubviews to be mutable, initialized as either empty or with the passed in initial views and update it accordingly when inserting or removing subviews.
There is another possible enhancement, so that every place where now self.subviews is used, to use self.arrangedSubviews and completely ignore any subviews which aren't array, but I'm not sure if that's a valid option in your opinions, so I haven't implemented that here yet.

Would love to hear feedback!
Cheers! 馃嵒

@@ -84,7 +84,7 @@ typedef NS_ENUM(NSInteger, OAStackViewAlignment) {
NS_ASSUME_NONNULL_BEGIN
@interface OAStackView : UIView

@property(nonatomic,readonly,copy) NSArray *arrangedSubviews;
@property(nonatomic,readonly,copy) NSMutableArray *arrangedSubviews;
Copy link

Choose a reason for hiding this comment

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

I think the idea was to replicate UIStackView API, and in Apple docs arrangedSubviews is NSArray.

You could keep public array as immutable, and provide private mutable array, of which immutable copy could be returned getter of arrangedSubviews.

Choose a reason for hiding this comment

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

Looks good except the different API. As Grubas7 said I'd keep a mutable version internally (with a different property name) and override the arrangedSubviews getter to return a (non mutable) copy of the mutable array. This will allow you to keep the arrangedSubviews property in the header as an NSArray

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@assafgelber
Copy link
Contributor Author

@bencallis Done!

@ZevEisenberg
Copy link
Contributor

+1 would love to see this merged.

@nsomar
Copy link
Owner

nsomar commented Nov 19, 2015

This looks fantastic, sorry for the late action, I will be merging it this weekend, thanks for the addition :)

@assafgelber
Copy link
Contributor Author

Awesome, thanks! 馃嵏

@@ -16,6 +16,7 @@

@interface OAStackView ()
@property(nonatomic, copy) NSArray *arrangedSubviews;
Copy link
Owner

Choose a reason for hiding this comment

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

Since this property is not used anymore I have removed it after rebasing.

Copy link

Choose a reason for hiding this comment

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

No, this property is used in line 76. It's provided to be consist with iOS9 API.

Copy link
Owner

Choose a reason for hiding this comment

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

yes its used in line 76 but the backing property and its Ivar are not needed. I only removed the property from the .m file, the .h file still has the readonly one.

@nsomar
Copy link
Owner

nsomar commented Nov 24, 2015

@agelber Great addition! I have rebased it and will merge it in a moment.
The only real change that I made was removing the old arrangedView in the class extension since its not used anymore.

Thanks for this PR!

@nsomar nsomar closed this Nov 24, 2015
@assafgelber
Copy link
Contributor Author

Cheers!

@dmakarenko
Copy link

Hi guys. Thanks for fixing the problem with the arrangedSubview property. Could you please bump the Podspec version so the fix can be accessible via the CocoaPods.
Cheers!

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.

None yet

6 participants