Added star empty state color, border color and border width #48
Conversation
star border colour and border width
| @@ -161,6 +167,38 @@ - (void)setFilledStarImage:(UIImage *)filledStarImage { | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| - (void)setFilledStarColor:(UIColor *)filledStarColor { | |||
| if(_filledStarColor != filledStarColor) { | |||
There was a problem hiding this comment.
Can you put a space after the if please? I'm planning on doing a major code style change on this library soon, but my OCD won't allow me to merge this without a space there 😆
| @property (nonatomic) IBInspectable CGFloat starBorderWidth; | ||
|
|
||
| @property (nonatomic, strong) IBInspectable UIColor *emptyStarColor; | ||
| @property (nonatomic, strong) IBInspectable UIColor *filledStarColor; |
There was a problem hiding this comment.
We shouldn't have a separate property for filled. The idea is to use tintColor whenever possible.
There was a problem hiding this comment.
suggested change done.
| @@ -62,6 +62,12 @@ - (void)_customInit { | |||
| _value = 0; | |||
| _spacing = 5.f; | |||
| _continuous = YES; | |||
| _starBorderWidth = 1.0f; | |||
| _starBorderColor = self.tintColor; | |||
There was a problem hiding this comment.
This shouldn't be set here, because if the tint color changes and we haven't set a custom border color, this would result in different colors for existing apps (which means we're braking API behaviour).
My suggestion is that we have a getter for starBorderColor that returns _starBorderColor if it's not nil, and self.tintColor otherwise.
This way if a developer already using this control updates the pod/code and doesn't care/know about custom border colors, his/her code won't break when (s)he tries to change the tint color.
What do you think?
| startBorderWidth = 0; | ||
| } | ||
|
|
||
| _starBorderWidth = startBorderWidth; |
There was a problem hiding this comment.
Instead of the if above, why not just _starBorderWidth = MAX(0, starBorderWidth)?
What the if should be doing is validating if _starBorderWidth != starBorderWidth, similarly to the implementation in the other setters.
| } | ||
| } | ||
|
|
||
| - (void)setStarBorderWidth:(CGFloat)startBorderWidth { |
There was a problem hiding this comment.
Typo in variable name: should be starBorderWidth and not startBorderWidth 🙂
| @@ -267,6 +308,8 @@ - (void)drawRect:(CGRect)rect { | |||
| CGFloat availableWidth = rect.size.width - (_spacing * (_maximumValue - 1)) - 2; | |||
| CGFloat cellWidth = (availableWidth / _maximumValue); | |||
| CGFloat starSide = (cellWidth <= rect.size.height) ? cellWidth : rect.size.height; | |||
| starSide = (self.shouldUseImages)? starSide : (starSide - _starBorderWidth); | |||
There was a problem hiding this comment.
Missing space before ?
Also, why is here exactly? (seriously, I'm asking out of ignorance 😆 and only proves how much I need to take another good look at my own code again)
|
Done |
irfangul92
changed the title
|
Thanks for those changes @irfangul92! I'm going to test this just to make sure I didn't miss anything and will include it in the next release! 😍 |
|
Hi @hsousa, do you have any intent to merge this pull request into master branch ? |
|
Sorry, yes! I've just been horrible at maintaining this... I'll try to push out a release this week with these changes! Thanks again for this! ❤️ |
|
Very nice @hsousa! Thank you for your quick response and for your beautiful library. |
This will allow to change the empty state color, so we will be able use separate colors for star fill state and empty state. In addition star border color and width properties are also configurable through IBInspectable. This will cover most of the UI variations if you want to stick with stars for rating.