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

Categories instead of NUI subclasses #8

Closed
supermarin opened this issue Dec 6, 2012 · 17 comments
Closed

Categories instead of NUI subclasses #8

supermarin opened this issue Dec 6, 2012 · 17 comments

Comments

@supermarin
Copy link
Contributor

Wouldn't it be a better idea to use categories on standard UIKit components, instead of subclassing them?

That way you wouldn't have to change each element's class in Interface Builder or code;
You'd use them as they were standard UIKit components.

@tombenner
Copy link
Owner

The original thinking was that some people may not want NUI applied to every UIKit component, as that might cause a conflict of some kind. I've had a couple requests for this change, though, and am beginning to lean towards categories.

Any thoughts on this? Are there any conflicts with using categories that anyone foresees?

@supermarin
Copy link
Contributor Author

I think unit tests are the best way to test out this behavior.

There are some alternatives to just monkey patching (overriding a method in a category), like method swizzling
I'll have to apply this to one project to see how it plays with custom drawings, etc

@kemenaran
Copy link

I fully agree on this : I think a lot of the static methods (surely not all of them) could be attached as categories to existing objects. Good candidates would be the methods that apply on an object passed as the first parameter.

Actually, when I first read the source code, I found it very simple and elegant, neatly organized, and without a bit of over-engineering. The only thing that bothers me is the use of static methods everywhere, which I think is not completely idiomatic :)

@supermarin
Copy link
Contributor Author

Yeah, I've completely refactored the NUISettings.m to use instance methods.
The only thing is that something's not good in the object setup. (see #6)

Hopefully tomorrow I'll be able to figure it out and make a pull req.

@tombenner
Copy link
Owner

I'm completely down for using fewer static classes; I'm relatively green to iOS dev, so I'm always happy to hear thoughts on how to make this more idiomatic.

I'm leaning more and more towards going with the category approach. If anyone else has thoughts on it either way, definitely feel free to chime in!

@supermarin
Copy link
Contributor Author

I haven't sent a pull request until the setup is done, but you can see how it looks here:
supermarin@60a60dd

@tombenner
Copy link
Owner

I've made a new branch that uses categories instead of subclasses. It's fairly close to being done; just about everything currently works visually except for UIBarButtonItems, but I'd like to do a lot more testing with different styling to make sure that nothing is being missed.

If you want to review the new setup, definitely let me know your thoughts on it. Some notes:

  • UI elements can be given custom style classes by setting a runtime attribute in IB (in the Identity inspector) named nuiClass with a value of something like Label:LargeLabel. Set the value to none to bypass NUI's styling.
  • Subclasses of UILabel and UIView are currently not styled by the Label and View style classes, as there are a number of subclasses of them in UIKit. UILabel is only styled when its superview is a UIView; otherwise many internal labels would be styled by Label (e.g. the label in UITableViewCellContentView). This isn't ideal, as it means subclasses like TTTAttributedLabel wouldn't be styled by the Label style class. Any thoughts on how to improve this?

@supermarin
Copy link
Contributor Author

The quickest one that comes to my mind is to use -nuiClass property.
It can be settable from interface builder (runtime attributes).

@n13
Copy link

n13 commented Dec 11, 2012

Ok I haven't even used this project - but I am super excited by it so forgive me if the below makes no sense.

Isn't it the case that you simply never want to style base classes? Because as you said a lot of the UIKit classes are composites and they'd all break if the base classes are styled?

Accidental application of styles needs to be avoided - It should just not be possible. So in that sense - why use categories at all? I love categories, but it seems like you should use them to implement the base styling functionality, but only actually ever apply styles to custom subclasses.

What is the advantage of applying styles based on the "nuiClass" field rather than the class name?

@supermarin
Copy link
Contributor Author

If nuiClass property is used, it shouldn't break any other UIKit components in my opinion.

@tombenner
Copy link
Owner

Agreed, @mneorr. @n13, the goal is to make the integration of NUI as quick and seamless as possible, and the use of categories best achieves this. UIKit should be styled by default.

Classes like UIButton and UIView are subclassed by and used within other UIKit components, so to prevent confusing styling inheritance, those classes will only be styled when they're not part of a composite UIKit component and aren't the ancestor class of another UIKit component.

@tombenner
Copy link
Owner

I've resolved all of the major issues I could find with the new categories setup and have merged it into master to prevent this divergence of branches from lasting too long. Currently, [NUISettings init]; needs to be called in main.m (see the new Installation instructions), as it both loads the style sheet and does the method swizzling. To use a custom .nss file, it can be replaced by [NUISettings initWithStylesheet:@"Blue.NUIStyle"];.

If you see any ways to improve this new setup, definitely let me know!

@supermarin
Copy link
Contributor Author

@tombenner awesome, I'm gonna try that out.
Sorry I haven't had time to work on the loading these days, too much of the other work.

Let me know if you would like to meet one day in SF and try to hack the loading back in place.

@raulraja
Copy link

@tombenner Just saw you added support for Categories. This is great news for us as the subclasses was the only drawback for us implementing NUI. I saw in the instructions about copying the folder with sources to the project. Do you have any plans on providing NUI as a .framework or lib that can be included in projects without the need to copy and compiling from sources?

@supermarin
Copy link
Contributor Author

@raulraja take a look at Cocoapods. Hopefully the pull request for the .podspec will get merged, and that would be the simplest way to integrate NUI.

@raulraja
Copy link

@mneorr thanks I have never used Cocoapods, looked at an article about it and looks cool. We actually use maven with https://github.com/sap-production/xcode-maven-plugin for both builds and dependency management. I was hoping I could install a .framework of NUI in our repo and have it as dependency for all projects where I plan to use it. We have builds and CI that depend on Maven and also there are Android submodules as part of this project so it's all part of parent reactor that builds Java and also iOS stuff. I'd really hate to clone this repo and include all those sources as part of our build not knowing when there is a newer version and all that. Worst case I'll create a .framework with the sources and push it to our repo manually but thought everyone out there may be facing this same problem if the way to install it is just via sources.

@tombenner
Copy link
Owner

We're now using categories (and have a .podspec), so I'm going to close this, but if there were any lingering issues discussed here that still seem relevant, feel free to open a new issue for them.

Also, @mneorr, apologies, had meant to get back to you much earlier about working on the loading. Things have been a little hectic for me, but feel free to shoot me an email if you ever want to do some hacking.

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

No branches or pull requests

5 participants