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.

Sign up for GitHub

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

Jump to bottom

Added star empty state color, border color and border width #48

Merged
merged 2 commits into from Mar 14, 2017
Merged

Added star empty state color, border color and border width #48

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f72d6e
Added star fill/empty colour
irfangul92 Nov 4, 2016
2ec0ab1
Added getter of BorderColor and code refactor
irfangul92 Nov 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions HCSStarRatingView/HCSStarRatingView.h
View file
Open in desktop
Expand Up @@ -39,6 +39,12 @@ IB_DESIGNABLE
// Optional: if `nil` method will return `NO`.
@property (nonatomic, copy) HCSStarRatingViewShouldBeginGestureRecognizerBlock shouldBeginGestureRecognizerBlock;

@property (nonatomic, strong) IBInspectable UIColor *starBorderColor;
@property (nonatomic) IBInspectable CGFloat starBorderWidth;

@property (nonatomic, strong) IBInspectable UIColor *emptyStarColor;
@property (nonatomic, strong) IBInspectable UIColor *filledStarColor;
Copy link
Owner

Choose a reason for hiding this comment

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

We shouldn't have a separate property for filled. The idea is to use tintColor whenever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

suggested change done.


@property (nonatomic, strong) IBInspectable UIImage *emptyStarImage;
@property (nonatomic, strong) IBInspectable UIImage *halfStarImage;
@property (nonatomic, strong) IBInspectable UIImage *filledStarImage;
Expand Down
49 changes: 46 additions & 3 deletions HCSStarRatingView/HCSStarRatingView.m
View file
Open in desktop
Expand Up @@ -62,6 +62,12 @@ - (void)_customInit {
_value = 0;
_spacing = 5.f;
_continuous = YES;
_starBorderWidth = 1.0f;
_starBorderColor = self.tintColor;
Copy link
Owner

Choose a reason for hiding this comment

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

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?

_emptyStarColor = [UIColor clearColor];
_filledStarColor = self.tintColor;


[self _updateAppearanceForState:self.enabled];
}

Expand Down Expand Up @@ -161,6 +167,38 @@ - (void)setFilledStarImage:(UIImage *)filledStarImage {
}
}


- (void)setFilledStarColor:(UIColor *)filledStarColor {
if(_filledStarColor != filledStarColor) {
Copy link
Owner

Choose a reason for hiding this comment

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

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 😆

_filledStarColor = filledStarColor;
[self setNeedsDisplay];
}
}

- (void)setEmptyStarColor:(UIColor *)emptyStarColor {
if(_emptyStarColor != emptyStarColor) {
_emptyStarColor = emptyStarColor;
[self setNeedsDisplay];
}
}

- (void)setStarBorderColor:(UIColor *)startBorderColor {
if(_starBorderColor != startBorderColor) {
_starBorderColor = startBorderColor;
[self setNeedsDisplay];
}
}

- (void)setStarBorderWidth:(CGFloat)startBorderWidth {
Copy link
Owner

Choose a reason for hiding this comment

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

Typo in variable name: should be starBorderWidth and not startBorderWidth 🙂

if(startBorderWidth < 0){
startBorderWidth = 0;
}

_starBorderWidth = startBorderWidth;
Copy link
Owner

Choose a reason for hiding this comment

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

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.

[self setNeedsDisplay];
}


- (BOOL)shouldUseImages {
return (self.emptyStarImage!=nil && self.filledStarImage!=nil);
}
Expand Down Expand Up @@ -245,15 +283,18 @@ - (void)_drawAccurateHalfStarShapeWithFrame:(CGRect)frame tintColor:(UIColor *)t
[clipPath appendPath:[UIBezierPath bezierPathWithRect:rightRectOfStar]];
clipPath.usesEvenOddFillRule = YES;

[_emptyStarColor setFill];
[starShapePath fill];

CGContextSaveGState(UIGraphicsGetCurrentContext()); {
[clipPath addClip];
[tintColor setFill];
[_filledStarColor setFill];
[starShapePath fill];
}
CGContextRestoreGState(UIGraphicsGetCurrentContext());

[tintColor setStroke];
starShapePath.lineWidth = 1;
[_starBorderColor setStroke];
starShapePath.lineWidth = _starBorderWidth;
[starShapePath stroke];
}

Expand All @@ -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);
Copy link
Owner

Choose a reason for hiding this comment

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

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)


for (int idx = 0; idx < _maximumValue; idx++) {
CGPoint center = CGPointMake(cellWidth*idx + cellWidth/2 + _spacing*idx + 1, rect.size.height/2);
CGRect frame = CGRectMake(center.x - starSide/2, center.y - starSide/2, starSide, starSide);
Expand Down