Skip to content
This repository has been archived by the owner on Jun 8, 2023. It is now read-only.

RBLViewController #46

Merged
merged 12 commits into from Jan 18, 2013
Merged

RBLViewController #46

merged 12 commits into from Jan 18, 2013

Conversation

Machx
Copy link
Contributor

@Machx Machx commented Nov 1, 2012

This is a pull request for RBLViewController as discussed in Issue #33 to get feedback before merging in. I started here with simply getting much of what Josh did in JAViewController working on Rebel.

I should also note that I had to add a NSObject swizzling methods category that seems like it really belongs somewhere else in foundation level code.

I am open to any changes that should be made or feature additions. I am already starting to use this class in my new project I am working on so I should get more ideas of things to add as time progresses. Thanks

@ghost ghost assigned joshaber Nov 5, 2012
@joshaber
Copy link
Contributor

joshaber commented Nov 5, 2012

Hi Colin! Sorry it's taken so long on this—I hope to give it a good review in the next day or two. Because oldjoshcodelolz.

@joshaber
Copy link
Contributor

joshaber commented Nov 5, 2012

Ah in the meantime, it looks like this doesn't merge cleanly as-is. Could you merge in upstream?

@Machx
Copy link
Contributor Author

Machx commented Nov 5, 2012

Okay i'll merge in upstream as soon as I can. Im not on the best network for that at the moment, so I'll wait just a little bit till later this afternoon.

@joshaber
Copy link
Contributor

Alright so a couple broad comments first.

  • I think the NSView -setNeedsLayout whatnot can go away since that's in AppKit as of 10.7.
  • All category methods/properties need to be prefixed with rbl_.

I'm inclined to think that the circumstances in which appearance methods should be called be left entirely to the user. In OS X land it seems waaaaaay too hard to know exactly what "appear" means universally.

dispatch_once(&onceToken, ^{
//swizzle swizzle...
[self rbl_swapMethod:@selector(viewWillMoveToSuperview:) with:@selector(custom_viewWillMoveToSuperview:)];
[self rbl_swapMethod:@selector(viewDidMoveToSuperview) with:@selector(custom_viewDidMoveToSuperview)];
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I don't mean to jump on @joshaber's territory here, but the indentation here looks like it's spaces and not tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes, there was a couple lines that had spaces instead of tabs. I don't know how I missed this. I'll fix it quickly. Looks like 4-6 lines that had spaces instead of tabs.

@joshaber
Copy link
Contributor

Oh man I'm really sorry Colin. This totally dropped off my radar. Next time give me a swift kick.

Could you merge master in so this merges cleanly? Then we should be ✨

@jwilling
Copy link
Contributor

💥 @Machx Helping Josh out here a bit, looks like you need to update the submodules as well as they seem to be pointing toward old commits.

@Machx
Copy link
Contributor Author

Machx commented Jan 16, 2013

there we go I think I got it all sorted out. let me know if there is anything else I need to do. Thanks!

@joshaber
Copy link
Contributor

💥 👶

joshaber added a commit that referenced this pull request Jan 18, 2013
@joshaber joshaber merged commit ae14d73 into github:master Jan 18, 2013
@jwilling
Copy link
Contributor

👏

@dannygreg
Copy link

One might say this pull is "sizzling" :trollface:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants