Fix/issue #209 autoPinEdge using RTL #211
Conversation
When you set the excluded edge to .right or .left, you would expect that the opposite would be pinned and not .leading or .trailing.
91192c7 to
fd3ee4f
Compare
toohotz
left a comment
There was a problem hiding this comment.
Hey there, it would be nice as well if you could also do the same for the -[autoPinEdgesToSuperviewMarginsExcludingEdge] since it follows the same principal in regards to the edge differentiation
|
@toohotz Done 👍 |
| if (edge != ALEdgeTop) { | ||
| [constraints addObject:[self autoPinEdgeToSuperviewEdge:ALEdgeTop withInset:insets.top]]; | ||
| } | ||
| if (edge != ALEdgeLeading && edge != ALEdgeLeft) { |
There was a problem hiding this comment.
You seemed to have reversed the edges here? Ie. ALEdgeTrailing going to ALEdgeLeading? Same goes for the ALEdgeRight and ALEdgeLeft.
There was a problem hiding this comment.
I changed the != to ==. So before, it was if the excluded edge is not leading, then pin to leading, now it is if the excluded edge is trailing, pin to leading. I changed it to == to make sure we're using either leading/trailing or right/left @toohotz
There was a problem hiding this comment.
@alyakan I follow now, it just seems a bit odd going from the non equality to equality and also the constraints originally are being added in a counterclockwise manner vs your changes now being clockwise so if I were used to making a reference to the second constraint out of the four that is returned before these changes, I would now end up with the opposite side constraint here. @tomidelucca @mickeyreiss @jjksam maybe you guys want to chime in here with your thoughts?
There was a problem hiding this comment.
Correct me if i'm wrong, but I believe that I still do follow the counter clockwise manner where I go by adding:
- Top (if excluded edge is not top)
- Leading/Left (if excluded edge is trailing/right)
- Bottom (if excluded edge is not bottom)
- Trailing/Right (if excluded edge is leading/left)
As for why I went to equality instead of inequality, it's just to simplify the if conditions because otherwise it would have been nested:
if (edge != ALEdgeLeading) {
if (edge == ALEdgeRight) {
// Add left edge
} else {
// Add leading edge
}
}
There was a problem hiding this comment.
My mistake, I was reading the conditionals with the assumption of the old code logic, makes sense now and avoiding the nested conditionals as well. 👍🏾
| if (edge != ALEdgeTop) { | ||
| [constraints addObject:[self autoPinEdgeToSuperviewMargin:ALEdgeTop]]; | ||
| } | ||
| if (edge != ALEdgeLeading && edge != ALEdgeLeft) { |
There was a problem hiding this comment.
Likewise here I believe you accidentally swapped their places as well.
|
Unit tests are failing in macOS. Margin related methods are only allowed in iOS. Can you fix them? The CI's log has relevant info! |
|
@alyakan I merged in my PR for the margins so you can pull in my changes and tuck your test methods underneath mine inside of the |
Rebasing to PureLayout:master
…ix/issue-#209-autoPinEdge-RTL
|
@tomidelucca Everything is good to go 👍 |
PR addresses issue: #209
Additions respect counter clockwise ordering of constraints