Vue Star Rating Component

Question

I am wondering if someone can do a code re"vue" specifically for my star rating component ([complete code available on github).

I am looking for feedback on best practices, any glaring code crimes I might have committed and bad code.

The code is also deployed to #Heroku here

Problems I saw with the code previous code are also included.

1. The partial star sometimes just gets the default colors, even if the DOM has the colors assigned via props.
2. Change in data (using Vue Dev Tools) does not re-render my star rating component. For the benefit for the code reviewers, I have hosted the component on Heroku, so if you have the Vue dev tools, you can test this too.

(削除) Edit - Inlined the code to follow the code-review standards. (削除ここまで)

Edit - 10/23 - I have updated the code since I first posted this and seem to have resolved all the issues with my previous code. The link to Github is here. Also the inlined code is now newer... I have added comments in the code, where I would love some feedback and comments.

 <template>
 <div>
 <!-- Is this use of dynamic component correct? -->
 <component v-for="n of this.totalStarsData" :key="n" :is="currentComponent(n)" :fillColor="ratingData - n > -1 ? fillColorData : baseColorData" :baseColor="baseColorData" :rating="ratingData"></component>
 </div>
 </template>
 <script>
// Can Star and PartialStar components be dynamically imported?
import Star from "./Star"
import PartialStar from "./PartialStar"
// Using a global variable like this - Good or bad?
let isPartialRendered = false
export default {
 name: 'star-rating',
 components: {
 Star,
 PartialStar
 },
 props: {
 totalStars: {
 type: Number,
 default: 5
 },
 rating: {
 type: Number,
 required: true
 },
 fillColor: {
 type: String,
 default: '#C00'
 },
 baseColor: {
 type: String,
 default: '#666'
 },
 },
 data(){
 return {
 // Any better way of converting props to data?
 // Also maybe naming convention best practices?
 totalStarsData: this.totalStars,
 ratingData: this.rating,
 fillColorData: this.fillColor,
 baseColorData: this.baseColor,
 }
 }, 
 methods: {
 currentComponent: function (count) {
 const int_part = Math.trunc(this.ratingData);
 if(count > int_part && isPartialRendered === false) {
 isPartialRendered = true;
 // Re-setting the isPartialRendered flag here, for cases where the Partial SVG is the last SVG
 if(count === this.totalStarsData){
 isPartialRendered = false;
 }
 return 'PartialStar'
 } else {
 if(count === this.totalStarsData){
 isPartialRendered = false;
 }
 return 'Star'
 }
 }
 }
}
</script>
<style scoped>
</style>

Answer 1

Converting props to data is a bad idea if it is not a copy of the prop because you might end up in a situation where your parent changes your prop and your component changes your data and then which is right? The component manipulating props is not good. Instead decide which is externally sourced (from parent) and which is internally manipulated, and then assign each to either prop or data.

Since your code in particular doesn't actually manipulate the data or props you are simply better off if you remove the data key completely, just leave it with props and you can access the values in precisely the same way using this.totalStarsData for example. You can clearly see that having a props key and manually entering the same info into data is ugly code anyway.

Having that global variable is unhelpful in my opinion, and probably bad practice since it goes against the decoupling of the component. Why not make it another prop with a default value of false?

• Shreerang: This is great feedback @Attack68 and something very similar to what I was already wondering about. #1 - One of the primary reasons I introduced data, was because I wanted to demo that the component could respond to data changes, and in the Vue Dev Tools, I was only able to modify the data values and not props. #2 - I was not very sure about that, but makes total sense. I am going to go ahead and make that change.
• Attack68: revisiting my own advice: since isPartialRendered is manipulated by your component it should not be a prop it should be data, but if it is something that all components use globallly and individual components can manipulate it then make it a data element of your parent instance and get your components to $emit events that will trigger the parent to manipluate it and it then gets automatically passed as a prop to all components at onec
• Shreerang: If I do place isPartialRendered in data and then use this.isPartialRendered in the method currentComponent won't it re-render the component every time I set the value of isPartialRendered? It might even throw the warning You may have an infinite update loop in a component render function.