Added missing superview margin with insets methods #210
Conversation
| [constraints addObject:[self autoPinEdgeToSuperviewMargin:ALEdgeLeading]]; | ||
| [constraints addObject:[self autoPinEdgeToSuperviewMargin:ALEdgeBottom]]; | ||
| [constraints addObject:[self autoPinEdgeToSuperviewMargin:ALEdgeTrailing]]; | ||
| return constraints; |
There was a problem hiding this comment.
Couldn't this method be now simplified by using the new autoPinEdgesToSuperviewMarginsWithInsets method and passing the ALEdgeInsets as zero? This way we could avoid duplicated code.
The same applies to the autoPinEdgeToSuperviewMargin: withInset: method.
There was a problem hiding this comment.
Agreed, -[autoPinEdgesToSuperviewMargins] makes sense to simplify with AlEdgeInsetsZero but that one differs from the -[autoPinEdgeToSuperviewMargin: withInset:] because here the relation does matter and there is no single method currently that does -[autoPinEdgeToSuperviewMargin: withInset: relation:]. Now if something like that is desired then yes, it could simplify things but that would affect backwards compatibility if those two methods were combined into a single one and the singular ones deprecated / removed. So for now, I can amend the changes for -[autoPinEdgesToSuperviewMargins]. Thanks for the feedback, well appreciated 👍
|
First of all, hi! I'm super excited to be here. I hope we can bring this library back to life now that we have a little more control over it. |
| inset = -inset; | ||
| } | ||
| ALMargin margin = [NSLayoutConstraint al_marginForEdge:edge]; | ||
|
|
There was a problem hiding this comment.
Just to be consistent, can we remove redundant newline?
There was a problem hiding this comment.
Of course, thanks for catching.
mickeyreiss
left a comment
There was a problem hiding this comment.
@toohotz thanks for being an awesome contributor! Much appreciated. Did you consider including some coverage for the new signatures in tests/readme/demo apps? I think that would be helpful for people who are trying to understand how these two new methods work.
|
@mickeyreiss Absolutely, tends to escape me to update the README but I'll do so and add in corresponding signatures in the tests along with the demo apps. Appreciate the feedback. 👍🏾 |
|
@mickeyreiss The signatures are included in the README they were just missing as someone had noted as well as the demo project has already signatures for margins so the understanding is there on how it is to be used just that it was missing 🤓. I added a few tests for the margins also. |
PR addressing issue #204.