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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UILayoutSupport support #203

Merged
merged 8 commits into from Sep 1, 2016
Merged

Add UILayoutSupport support #203

merged 8 commits into from Sep 1, 2016

Conversation

TimothyChilvers
Copy link
Contributor

Hope you don't mind my taking a stab at this. It's based on the examples in #95 and admittedly it doesn't quite match the framework pattern, but it's here if you want it.


init(_ multiplier: CGFloat, _ constant: CGFloat) {
Copy link
Owner

Choose a reason for hiding this comment

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

Did you make this public for your testing or is there a reason for those changes?

@robb
Copy link
Owner

robb commented Apr 7, 2016

Thanks for the pull request! 💖

Would you mind adding some tests that also exercise the new API?

@CVertex
Copy link

CVertex commented Jul 12, 2016

Where's this at? Would love to see this in my favourite AutoLayout library :)

@TimothyChilvers
Copy link
Contributor Author

TimothyChilvers commented Jul 13, 2016

I've been using it for months now and it's been fine, I'd like to add the tests and change the names to match styles that have been discussed elsewhere…. But I haven't done it. I'll see if I can do that by the end of the week.

@CVertex
Copy link

CVertex commented Jul 13, 2016

great! Can't wait! :)

@orta
Copy link
Collaborator

orta commented Aug 19, 2016

@TimothyChilvers this looks good 👍 - it's worth trying to get this tested and merged

@TimothyChilvers
Copy link
Contributor Author

All right, all right… I'll pick it up again now.

@TimothyChilvers
Copy link
Contributor Author

Is my approach preferred over #212 ?

@orta
Copy link
Collaborator

orta commented Aug 19, 2016

@robb that opinion is on you I'm afraid ^

They should give layout guide lengths. They don’t give layout guide lengths. I dislike them intensely.
@TimothyChilvers
Copy link
Contributor Author

TimothyChilvers commented Aug 19, 2016

I added tests, but I'm not happy with them. The reason being I can't make the UILayoutGuide of the view controller have a non-zero length just using UIKit components straight off. I only spent an hour or so fiddling with it, so I might have missed something. I have another branch which stubs the view controller and layout guide classes, but the compiler warnings for availability are a nightmare.

If the tests are adequate (In my mind they're not the greatest), feel free to merge. If someone wants to have a go at populating the layout guide with a length, also be my guest. I'll put some time aside on Monday if no further action has been taken.

@TimothyChilvers
Copy link
Contributor Author

So…

Having tried subclassing UIViewController and giving custom UILayoutGuide subclasses to the guides and failed, and failing to populate the UIKit supplied top and bottom layoutGuide objects with properties, and finally with UILayoutSupport not being equatable / internal behaviour and constraint generation being unreliable, I'm calling this as the limit of what I can achieve with the tests.

@orta
Copy link
Collaborator

orta commented Aug 28, 2016

Alright, given the comprehensive docs / tests on this PR, I'm gonna merge it tomorrow unless I hear a good to not

}

}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: missing new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hate that too. Fixed.

@orta
Copy link
Collaborator

orta commented Sep 1, 2016

Hah - nice. OK, let's get this in.

@orta orta merged commit 0bd4be0 into robb:master Sep 1, 2016
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

4 participants