Comments (24)
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 theRatingContainer
component, since what theRatingContainer
does is expose that API and also normalize the values for consumption from theRating
. We could let people create their ownRatingContainer
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.
Related Issues (20)
- Initial Rating returns null only renders once
- on version 2.0.1 ReactRating-component onChange-event seems to return always value of initialRating HOT 15
- Error during installation on jenkins HOT 2
- emptySymbol is not hidden when fullSymbol is displayed HOT 9
- Cannot read property 'type' of undefined
- Method comments are switched HOT 1
- Misalignment HOT 4
- Sorry HOT 1
- All touches, even scroll/swipe gestures trigger "onClick" HOT 8
- Warning: Using UNSAFE_componentWillReceiveProps in strict mode is HOT 10
- Warning: Using UNSAFE_componentWillReceiveProps in strict mode is not recommended and may indicate bugs in your code. See https://reactjs.org/link/unsafe-component-lifecycles for details. HOT 5
- Feature request: Lighter DOM Size HOT 1
- Doesn't work with react 17 HOT 1
- Selection requires double click if onClick method provided HOT 3
- How to add the validation for the rating component HOT 1
- Add option to remove default style
- mixed symbol but react component HOT 1
- Package should be marked as deprecated and a replacement fork nominated HOT 2
- React 18: TS2786: 'Rating' cannot be used as a JSX component. HOT 9
- `@types` dependencies should be in `devDependencies`
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from react-rating.