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

Cleaned up some of the code relating to default colors #27

Merged
merged 6 commits into from May 17, 2014
Merged

Cleaned up some of the code relating to default colors #27

merged 6 commits into from May 17, 2014

Conversation

squarefrog
Copy link
Contributor

NOTE: please don't feel like you have to merge this. It's unfinished, I'm just sending you this PR so you can see one way of tackling some of the code cleanup.

I have removed all the setDefaultXX color methods, and replaced them with a simple ternary operator. Have a look and see what you think. If you think you like it I'll finish it up.

But again, don't merge this this is just an option for you!

@tandavas
Copy link
Member

Looking great 👍 Please do continue working on it. Thank you, Paul.

@squarefrog
Copy link
Contributor Author

Does it make sense to you? If a user wants to revert to the default color, they can always [button setButtonColor:nil];

@herinkc
Copy link
Member

herinkc commented May 15, 2014

@squarefrog Won't the default color be applied anyway without doing [button setButtonColor:nil]?

@squarefrog
Copy link
Contributor Author

Yes - if a user never sets a value to the buttonColor property it will just use the default.

Just as a marker for anyone else that views this, this was originally discussed in #17

@squarefrog
Copy link
Contributor Author

OK I'm done. You can clone a copy of my fork and check it out. If you're happy, then it can be merged in if you'd like.

I discovered a bug in the previous code where setDefaultShadowHeight
would not work correctly with circular style. This was a result of style
not actually being set yet.

Also cleared the deprecated method warnings.
@squarefrog
Copy link
Contributor Author

I spoke too soon! I just discovered a bug with HTPressableButtonStyleCircular brought about by my previous changes in this pull request. It seems so obvious now, but if you look at the init method:

- (instancetype) initWithFrame:(CGRect)frame buttonStyle:(HTPressableButtonStyle)style
{
    if (self = [super initWithFrame:frame])
    {
        [self setDefaultShadowHeight];
        [self setStyle:style];
    }
    return self;
}

the default shadow height method wants to know the button style, which isn't set yet. Initially I just swapped the two method calls over, but setStyle: also calls createButton. So I've adapted the init method to:

- (instancetype) initWithFrame:(CGRect)frame buttonStyle:(HTPressableButtonStyle)style
{
    if (self = [super initWithFrame:frame])
    {
        [self setDefaultShadowHeightForStyle:style];
        [self setStyle:style];
    }
    return self;
}

Now all is well! This is also a stark reminder that #30 should be the top of the to-do list next. I have testing experience, so I'm happy to walk you guys through it.

@herinkc
Copy link
Member

herinkc commented May 17, 2014

@squarefrog Looks really clean! One question though, why do we have to keep the part #pragma mark - Deprecated Methods?

@squarefrog
Copy link
Contributor Author

Good questions. So if we just remove those old methods, and someone is using them in their project, then it will produce unrecognised selector errors. Setting them as deprecated means it warns them that the method is going away soon so gives them a chance to remove reliance on those methods.

That way in a future release we can delete them all together and we've given people enough warnings that the method is due to go away.

@herinkc
Copy link
Member

herinkc commented May 17, 2014

Oh, that makes sense. So it's like warning before completely removing it. Then here we go!

I'll be editing the README and let's move to issue #30.

herinkc added a commit that referenced this pull request May 17, 2014
Edit default methods and added initialiser method
@herinkc herinkc merged commit 7665d9b into Famolus:master May 17, 2014
@tandavas
Copy link
Member

I was away for a few hours, and both of you are already did an awesome work! Thank you @squarefrog. This looks extremely nice 😄 👍

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

3 participants