Skip to content

Conversation

@javitrujillo
Copy link
Contributor

  • After commit 04a3a0b it behaves wrong when receiving certain values like ALEdgeTop or ALEdgeBottom.

- After commit 04a3a0b it behaves wrong when receiving certain values like ALEdgeTop or ALEdgeBottom.
@toohotz
Copy link
Member

toohotz commented Mar 22, 2018

So looking into the issue more and also the #211 has issues of it own when I ran the demo 9 where it uses the [self.greenView autoPinEdgesToSuperviewMarginsExcludingEdge:ALEdgeBottom];

So with the first issue, when you make a call for -[autoPinEdgesToSuperviewMarginsExcludingEdge] where the excluding edge is either ALEdgeTop and ALEdgeBottom, using your code just for example, you're using ALEdgeLeading and ALEdgeTrailing to handle the sides of the view. That can impose an issue if one intended to use the ALEdgeLeft and ALEdgeRight here so it made me think about this method and its use case because as a user I would either like to be aware that Leading / Trailing is being used so that if I handle RTL languages, I would individually have to setup my three constraints.

The second issue I came across purely because your PR made me look at the constraint logic I somewhat questioned also and actually seeing it in action I noticed that the logic doesn't seem to do what it was intended to. Easily tested this by first setting a fixed width on the green view in demo 9 by doing
[self.greenView autoMatchDimension:ALDimensionWidth toDimension:ALDimensionWidth ofView:self.yellowView withMultiplier:0.5];

Imgur
The yellow view nor the green view were displaying which led me to believe there were constraint issues. Breaking apart the constraint into the three in individual calls by doing

[self.greenView autoPinEdgeToSuperviewMargin:ALEdgeLeading];
[self.greenView autoPinEdgeToSuperviewMargin:ALEdgeTop];
[self.greenView autoPinEdgeToSuperviewMargin:ALEdgeTrailing];

Imgur

I guess I might be digressing a little bit but usage of this API 90% of the time will take care of what most will use it for but for those that will accommodate RTL then we should let them be aware that they should probably create the constraints individually.

@javitrujillo javitrujillo force-pushed the fix_autoPinEdgesToSuperviewMarginsExcludingEdge branch 2 times, most recently from a40bd76 to 967a969 Compare March 22, 2018 08:38
@javitrujillo
Copy link
Contributor Author

javitrujillo commented Mar 22, 2018

Thank you for your response!

I missed the autoPinEdgesToSuperviewEdgesWithInsets:excludingEdge: method. I changed the code to apply the fix to this one too. With this, the demo 9 is fixed.

FYI, we were having trouble with CTAssetsPickerController, which depends on PureLayout (the layout became broken after pointing to the latest PureLayout commit).

screen shot 2018-03-22 at 9 47 34

Now, it's fixed:

screen shot 2018-03-22 at 9 46 07

@javitrujillo javitrujillo changed the title Fixed issue on autoPinEdgesToSuperviewMarginsExcludingEdge method Fixed issues on *excludingEdge: methods Mar 22, 2018
@toohotz toohotz self-requested a review March 26, 2018 19:21
Copy link
Member

@toohotz toohotz left a comment

Choose a reason for hiding this comment

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

Thank you for making the additional changes. I will update the README per our discussion here so that users are aware that when the excluding edge is either ALEdgeTop or ALEdgeBottom that leading and trailing constraints will be used.

@toohotz toohotz merged commit 6f66318 into PureLayout:master Mar 26, 2018
@javitrujillo
Copy link
Contributor Author

Thank you!

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.

2 participants