React-rating v1 about react-rating HOT 24 CLOSED

Wow! Nice to hear from you @mkarajohn! If I am not wrong and I remember correctly it was almost finished. Let me review it this weekend and I will tell you something.

Probably we will have to "merge" some nice commits from current version.

A major React version wakes anyone up instantly 😉.

from react-rating.

And we are live! 🎉

from react-rating.

I have been thinking about callbacks... What if we restrict callbacks to just those related to actual values? I identify them by callback parameters. If the callback does not need the value to be passed then the event is tied to the container and not the value. What do you think?

Even more, if the callback is not tied to the actual value, we could just propagate the props to the top element (Rating) and let React deal with all the default props, onMouseEnter and onMouseLeave included.

One drawback... if we use standard callback names for value related events, like onClick, we are overwriting the container one. I mean, how do we know if we have to propagate onClick? By function signature? We are preventing users to attach an onClick callback to the Rating as a whole (useful?, I don't know 😉).

from react-rating.

So, tell me if I understand this correctly, you mean something like having these propagated to the DOM container of the component?

In that case, how do we know that the user meant for these events to be handled by the container DOM element and not by the symbol DOM elements?

I think the best approach would be to sit down and think about what someone who uses a star rating component actually cares about and focus on that.

I think a good answer is 'he cares about a value'. That means that he probably mostly needs ways to know about the state of the value. The star rating component will be living inside some bigger component that in turn needs to know about what is going on with the value of the star rating component. That's why we are trying to cover usecase scenarios for this need with onChange, onHover and onClick.

Now, would someone who uses a star rating component care much about DOM events related to the star rating DOM element? Well, he could, but we know that he can latch to the DOM element of the star rating component anyway (he can have a querySelector() inside the componentDidMount of the parent component of the star rating component, for example).

TL;DR

I think I agree with

What if we restrict callbacks to just those related to actual values?

This component is already so flexible with the range of values it can provide and display, for me that is its strong point. I know that providing an API for interacting with the value is important. I don't know if providing an API for interacting with the underlying DOM is very relevant.

Also, sorry for the absence, but the last couple of weeks I have been really busy with work and assignments; a lot of stuff came out of nowhere 😅 . I have not forgotten about this, I will continue work on it with the first opportunity. I think this weekend I should have free time.

from react-rating.

So, tell me if I understand this correctly, you mean something like having these propagated to the DOM container of the component?

You got it right! Not only for callbacks/events but for any prop that we do not explicitly use.

In that case, how do we know that the user meant for these events to be handled by the container DOM element and not by the symbol DOM elements?

Exactly. That was my concern. How to discriminate those props that should be propagated to the container.

I think the best approach would be to sit down and think about what someone who uses a star rating component actually cares about and focus on that.

I think a good answer is 'he cares about a value'.

Then we agree 👍. If we come up with a callback that does not provide the value as a parameter maybe we are facing a code smell.

Also, sorry for the absence, but the last couple of weeks I have been really busy with work and assignments; a lot of stuff came out of nowhere 😅 . I have not forgotten about this, I will continue work on it with the first opportunity. I think this weekend I should have free time.

No pressure. We are in this because we enjoy it 🙂.

from react-rating.

I implemented placeholder rating here 8108f87 and I also updated the readme 8b9215d. Have a look whenever you can :)

from react-rating.

Looks good! I only quick-fixed the default initialRating and placeholderRating value. It should be undefined instead of 0. Actually 0 is a valid value.

If you are OK with everything I will make a quick check and we are ready to make the release.

from react-rating.

from react-rating.

@djkwagala Originally I took the idea of start, stop, step from Python range. The only difference is that I moved the value exclusion from stop to start. I mean, start excluded and stop included (start, stop] instead of the python version [start, stop).

So you can have ranges including 0 as value, like:

<Rating start={-5} stop={4} />

This will include the following values:

[-4, -3, -2, -1, 0, 1, 2, 3, 4]

Check this jsfiddle.

from react-rating.

@dreyescat Hm, I am not sure I would consider 0 a "valid" value (like, a value a user could input) either. It could be a valid value if the user sets the initialRating to something < 0. However, come to think about it, it is not common to have negative values for star based ratings (at least I have not seen one), so maybe disallow negative initialRating altogether? Also, the code, currently does not allow returning non-positive values, by user input, because if the user even tries to set 0, by moving the mouse on the leftmost edge of the rating component, he will only be able to set it to the lowest positive value.

Also, I think undefined is not properly handled to be translated as 0 for the displayValue and is dependent on type casting right now (the undefined leads to some NaN value, whch ends up as 0), so if we are to keep undefined let's handle it properly.

tl;dr, the only 'valid' values the user can set currently are positive ones. We should update the code if it is to handle negative values, and also properly handle undefined

from react-rating.

@mkarajohn Allowing 0 and negative numbers as valid values is something I introduced from the beginning just because I liked to see the Rating as a range of values, influenced by the Python version of range. However, it is not the first time that it shows up. Maybe it is more a burden than a feature... 😉. Let me think about it.

Regarding undefined initialRating/placeholderRating I see it as a different issue. What is the expected value for a non-rated Rating?

from react-rating.

@mkarajohn BTW, since you introduced the RatingContainer layer I have started to see things different (sepparation of concerns). I see Rating as the normalized version of the RatingContainer . The former takes care of a (0..n] range of values while the latter (the actual Range Rating) takes care of the ranging (start, stop, and step). I mean, we could export both. Use Rating for the standard 5-star rating and the ranged one for more specialized ratings. We could have many special rating as far as we know how to map them to the normalized one 😃.

from react-rating.

I am back 😅 . Bumping this to see how we proceed? Cheers

from react-rating.

@dreyescat Yello and happy holidays!

So, I finally have quite some free time, now with the holidays and all, and I was wondering if you'd like to catch up with v1 and release it or have you lost interest?

In the latter case I will proceed with forking this repo and release the current v1 (with any necessary reworks since the last time I worked on it).

Cheers!

from react-rating.

Happy Xmas @mkarajohn!

Please. Go ahead. You can do the rework you need on v1 branch and merge it back to master. There will be some commits to take care of since we started v1, like Add touch events for mobile devices (edba58b).

Once it is merged to master we just push the release button. Even better. I would like to show you the release process and the commands/scripts I use. Just in case you need to release a new version without me 😉 . I like the idea of this project having more than one full maintainer.

from react-rating.

Ok, I made some final commits. Please do check it again before we merge into master, to ensure we are ok with the current API.

For a later stage, I am also thinking what you had said in an earlier comment, about exporting both the Rating and the RatingContainer component, since what the RatingContainer does is expose that API and also normalize the values for consumption from the Rating. We could let people create their own RatingContainer

from react-rating.

pinging @dreyescat just in case you did not get a notification from the previous comment 😅

from react-rating.

Ok, I made some final commits. Please do check it again before we merge into master, to ensure we are ok with the current API.

Ok. I did a quick review. You can merge it to master.

There is one thing that I would like to recheck once everything is merged to master. It is regarding my stubborn idea of having an undefined value for no value and consider 0 a valid value 😅. I have seen your commit 2d37d85 uses 0 as a special value for no displayed symbols. You are not the only one that considers 0 not valid rating value. A good contributor @djkwagala also questioned this idea in a previous comment. Let's keep this change and assume 0 as not valid for now 👍.

For a later stage, I am also thinking what you had said in an earlier comment, about exporting both the Rating and the RatingContainer component, since what the RatingContainer does is expose that API and also normalize the values for consumption from the Rating. We could let people create their own RatingContainer

Yep! I see Rating as the core one. The normalized one. Then, for example, the one that allows to set a range (additional start, stop, and step properties) could be the RangeRating.

from react-rating.

Well done @mkarajohn! 👏.

I added some minor fixes. I don't see anything that prevents us to release v1!

I have seen you already have a npm user @mkarajohn. Would you mind if I add you to react-rating as a core maintainer? You will be able to publish new releases... Like the current one 😉

from react-rating.

@dreyescat yeah, sure!

from react-rating.

You are officially core contributor! Let's celebrate being you the one who releases this major version!

I've just documented the release process. Nothing fancy. Just the three commands I use everytime I create a new version. Let me know if you have any issue.

from react-rating.

Cool, sorry for the long wait, I was off.

Ok, I am releasing, wish me luck!

from react-rating.

@dreyescat actually, I 'd rather not release yet, as I am having this issue with the demo page, which I have no idea what is causing it. Any ideas?

UPDATE: I am stupid, I was on a lower version of react with a different file structure. All is working now that I upgraded

from react-rating.

Very good job @mkarajohn!

Now, closing this issue...

BTW, the release deserved such a nice npm badge 😉.

from react-rating.