Skip to content

Conversation

@DanSkeel
Copy link

Before the fix custom images were scaled to match fixed 44 point height.
Now intrinsicContentSize depends on size of custom image to prevent image scaling.

Hi. I hope I don't miss anything, but it works for me.

Before the fix custom images were scaled to match fixed 44 point height.
@DanSkeel
Copy link
Author

Probably it's a bad idea, because a tappable area may become less than 44 points. So there should be other solution for custom sized "stars"

@DanSkeel DanSkeel closed this Aug 26, 2016
…l custom images.

Now custom images won't scale if size of view is bigger than side of image
@DanSkeel DanSkeel reopened this Aug 26, 2016
@DanSkeel
Copy link
Author

  1. Images less than 44 pt won't scale
  2. height of ratingView will match the size of custom images but it won't be less than 44 pts.

Copy link
Owner

@hsousa hsousa left a comment

Choose a reason for hiding this comment

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

Sorry for only looking at this now! 😞

Code overall LGTM, just have some minor tweaks and should be good to go!

- (CGSize)intrinsicContentSize {
CGFloat height = 44.f;
return CGSizeMake(_maximumValue * height + (_maximumValue-1) * _spacing, height);
CGFloat imageSide = self.emptyStarImage.size.height;
Copy link
Owner

Choose a reason for hiding this comment

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

Total nitpick here, but could we call this imageHeight?

CGFloat availableWidth = rect.size.width - (_spacing * (_maximumValue - 1)) - 2;
CGFloat cellWidth = (availableWidth / _maximumValue);
CGFloat starSide = (cellWidth <= rect.size.height) ? cellWidth : rect.size.height;
if (self.emptyStarImage) starSide = self.emptyStarImage.size.height;
Copy link
Owner

Choose a reason for hiding this comment

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

We should rely on self.shouldUseImages for this for consistency.

@hsousa
Copy link
Owner

hsousa commented Nov 5, 2016

Oh, forgot to mention in the review but: Thanks for this! 😄

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.

2 participants