4umo / react-native-stars

a versatile react native star review component with half star compatibility and custom images, star sizes, star count, star spacing, and value display.
MIT License
91 stars 19 forks source link

Rating mode doesn't handle arbitrary fractional values correctly #8

Closed peacechen closed 6 years ago

peacechen commented 6 years ago

rating mode only handles integer and 0.5 value increments properly. This is the code that does the evaluation: https://github.com/Extrct/react-native-stars/blob/master/index.js#L162

If the rating prop passed in is 4.5, it shows 4.5 stars as expected. However if the rating prop is 4.7, it shows 4 stars due to the logic on that line.

4umo commented 6 years ago

This is intended behavior for now. Down the road, I'm considering using one long Touchable element to handle 'continuous' rating values so a user can slide their finger to achieve a more granular rating. Then again, psychologically, people tend to loathe additional complexity in choices that could otherwise be simple ( eg: I don't want to think about whether my meal was 3.68 stars or 4.15 stars, I'd rather just say 4). But there may be applications for continuous rating that make sense, so it's still in the roadmap.

peacechen commented 6 years ago

The use case for supporting non-0.5 increments goes something like this:

The rating prop passed in to RN stars was fetched from another source. Maybe it's an average of all ratings stored server side. Let's say this value is 4.7.

4.7 is rendered as 4 stars which isn't accurate. It should be either 4.5 or 5 depending on rounding strategy.

The user is allowed to add their own rating in increments of 0.5, which is sent back to the server and averaged in to the overall result.

In that use case, the app would need to add boilerplate code to round the rating prop to 0.5 increments. IMHO that boilerplate should be in RN stars.

4umo commented 6 years ago

This functionality is already implemented - in the docs it's referred to as 'display mode'. All you have to do is pass something to the value prop. It might not work with custom components (the type you just added support for), I'll take a look at that when I get a chance.

peacechen commented 6 years ago

Display mode (value prop) rounds properly. It's the rating mode that is broken for this use case.

Or do you mean that both rating and value props should be passed in?

4umo commented 6 years ago

Hmm I still don't really see the value in the component defaulting the rating to the rounded aggregate value- if you're using the rating prop, the component is in selection mode and therefore won't display the aggregate/fractional value. Perhaps in the future I'll add a hybrid mode - which initially displays the proportion of stars passed to the value prop, but on hover converts to selection mode and rounds to the nearest half star or something.

Is that the use case you're looking to cover?

4umo commented 6 years ago

Regardless, It's an easy add and doesn't interfere with traditional use cases. I'll get to it as I'm doing a cleanup/refactor soon.

peacechen commented 6 years ago

Yes that is essentially the use case. Currently selection mode is able to display half stars, but only for rating values that are exactly increments of 0.5. I agree that the actual selection should continue to be limited to integer values (full star increments instead of half). Selecting half a star would end up being inconsistent due to touch granularity and confuse the user.

PS: Since the prop is named rating, it would be clearer in the documentation to refer to it as Rating mode. It took me a while to figure out what was what.

4umo commented 6 years ago

I'm confused - the module currently supports half star selection, and provided the dimensions of the rendered component are large enough, it works fine in terms of accuracy of touch position. I think this feature should remain as it is one of the things that sets this module apart from other RN star rating modules.

I agree that rating prop name should be changed, but I was thinking more along the lines of 'initialValue' or something- asrating shouldn't be used for anything else.