DoSomethingArchive / LetsDoThis-iOS

:iphone: iOS source code for DoSomething: Take Action on the News
http://app.dosomething.org
MIT License
2 stars 0 forks source link

Set custom font for TSMessages #110

Closed aaronschachter closed 9 years ago

aaronschachter commented 9 years ago

Following docs in https://github.com/KrauseFx/TSMessages causes build to fail: https://github.com/KrauseFx/TSMessages/issues/232

aaronschachter commented 9 years ago

Actually running a pod update has borked TSMessages completely, and the build won't run anymore as is: https://github.com/KrauseFx/TSMessages/issues/234. This is a pretty good argument against liberal use of Cocoapods

aaronschachter commented 9 years ago

Worth thinking about ditching this library eventually, as it's no longer in active development (or up to date docs...) https://github.com/KrauseFx/TSMessages/issues/242#issuecomment-124194456

aaronschachter commented 9 years ago

Need custom json file -- https://github.com/KrauseFx/TSMessages/issues/232#issuecomment-127569875

tongxiang commented 9 years ago

@aaronschachter Trying to wrap my head around how we’re using TSMessages in the app. 1) Why did we subclass TSMessage as LDTMessage? 2) In some cases, we’re using both TSMessage and LDTMessage (i.e., in LDTCampaignDetailViewController. Why are we using both of them? 3) To change the font as described here, can I add the setting to the LDTMessage subclass?

aaronschachter commented 9 years ago
  1. Specifically for having methods like displayErrorMessageForError:. Although now that I think about it, this probably should have been implemented as a Category and I didn't know what those were yet.
  2. Yeah, there's really no difference between the two classes -- the only reason I created the class was for helper methods. I suppose there's an advantage of having our own class if we ever wanted to ditch TSMessages completely and swap out the LDTMessage subclass, but it might be cleaner to just use the Category for now and kill LDTMessage completely...
  3. So this [TSMessageView addNotificationDesignFromFile:@"customNotifDesign.json"]; is a class method. It's not being called an object we create but just generally from the TSMessageView. class. To be honest I'm not completely sure what the best practice is here, or the inner workings of the TSMessageView class. (feel free to check out the source code though) Might have to read up on it but I would start by adding the line to our code in AppDelegate.m where we set up things to see if it works on all screens. If not it may need to be added screen by screen in styleView
aaronschachter commented 9 years ago

@eroth Might not be able to answer this now, but just checking: Does using the TSMessage category sound like a better approach than our custom class (where I'm just using it for helper methods) ?

eroth commented 9 years ago

What would it be a category on? A UIView? Categories are to augment existing classes, to extend their functionality.  In this case, since this is something entirely different, I think it makes was more sense to have it as a custom class, since I think (I haven't really looked at it) it does a lot of things: presenting a message, dismissing it, etc.

On Sep 16, 2015, at 02:58 PM, Aaron Schachter notifications@github.com wrote:

@eroth Might not be able to answer this now, but just checking: Does using the TSMessage category sound like a better approach than our custom class (where I'm just using it for helper methods) ? — Reply to this email directly or view it on GitHub.

tongxiang commented 9 years ago

I'm not sure I understand.

@eroth--I think @aaronschachter's proposing a category extending the TSMessage class. Does that change things?

aaronschachter commented 9 years ago

Yes thats what i mean. Specifically to add the displayErrorMessageForError: method to the TSMessage class

eroth commented 9 years ago

Sorry, hadn't looked at project—just did.  Oh, I see what you did.  LDTMessage is kind of unnecessary, but if you wanted to keep it you shouldn't do the error processing/creation in +(void)displayErrorMessageForError:(NSError *)error.  That error object should be created somewhere along the chain where the API failure is registered as a result of the AFN call, and then bubbled up.  LDTMessage should just display any error it's given—it shouldn't create an explicit error for us when that method is called.

On Sep 16, 2015, at 04:44 PM, Aaron Schachter notifications@github.com wrote:

Yes thats what i mean. Specifically to add the displayErrorMessageForError: method to the TSMessage class — Reply to this email directly or view it on GitHub.

tongxiang commented 9 years ago

Thanks, @eroth. Should we continue using the subclass LDTMessage? Should we turn it into a category of TSMessage?

eroth commented 9 years ago

You could argue either way—I'd just keep it.

On Sep 16, 2015, at 05:43 PM, Tong Xiang notifications@github.com wrote:

Thanks, @eroth. Should we continue using the subclass LDTMessage? Should we turn it into a category of TSMessage? — Reply to this email directly or view it on GitHub.