Add StarRating

[Chalarangelo]

A star rating component:

• A class component with an inline functional component.
• Uses state.
• Binds event listener callbacks to its own context.
• Has very little CSS-in-JS (unopinionated mostly).
• Passes own methods to children via props.

[skatcat31]

I'm not certain where to begin the review on this. I know for a fact this is not the way we want to be doing this. There are WAY to many handlers on the individual stars, an inline in the render function, derived state changes, and many other issues.

So I guess I'll start with the explosion of functions in it? One render for 5 stars has 12 functions made for every render(the star, onMouseOut, and 2 functions for each star of onMouseEnter, and onClick). It should ideally be one or none(if you can find a clean way to bubble the hover event and calculate which star it's over, but that's a much harder task).

You can easily do this with a surrounding onClick handler that will let the event bubble up and identify the star based on the event target.

The stars don't need an inline class in the render, it should instead be a class on the parent as a property or contained inside a closure as a private from an IIFE that returns the StarRating so that only the parent can create them. They should have as few handlers as possible created due to the constant re-rendering of the parent due to cascading state.

If you could drill this down to only 1 binding per child then it would be much closer to how it should perform, however I believe the version with 0 bindings would be preferred in a production environment. That is however a different endeavor.

Otherwise you should note the performance implications of having many of these on a single page.

Since complicating this further makes it a very long snippet, I'm voting for noting the performance implications myself

[atomiks]

We should be using the class properties proposal (autobinding) instead of specifying a constructor.

[Chalarangelo]

@atomiks Care to explain a bit? I am not that great with React just yet.

@skatcat31 Granted, the onMouseEnter handlers are very complicated to implement on the parent component, but the onClick handler can be easily improved, I'm gonna give that a shot in a little bit.

As far as the inlined <Star> component is implemented, I could use some suggestions on what to do with it. It seemed like a good idea at the time, but it's probably better if it's placed elsewhere to make things slightly cleaner and more performant. I'm open to ideas on this one.

[Chalarangelo]

@skatcat31 The onClick handler is all set - from 5 * 2 + 1 methods, we are now to 5 + 2 methods, which is a marked improvement. If someone is smart enough to code a hover method that figures out which element is hovered over that would be a really good step forward. 😉

[atomiks]

Here..

• Use class properties proposal
• Move Star outside (why was it in render method?)
• Differentiate props.rating and state.rating. Props would be the initial rating (I guess the average from all ratings), state would be the user's own rating?
• Simplify the array map & make it static, slight perf improvement
• Remove star-id (invalid attribute, has no purpose?)
• Rename marked to isMarked?
• Rename starId to id?

const oneToFiveArray = [...Array(5)].map((_, i) => i + 1)

function Star({ id, isMarked, onHover, onRate }) {
  return (
    <span
      style={{ color: '#ff9933' }}
      onClick={() => onRate(id)}
      onMouseEnter={() => onHover(id)}
    >
      {isMarked ? '\u2605' : '\u2606'}
    </span>
  )
}

class StarRating extends React.Component {
  state = {
    rating: null,
    selection: 0
  }

  hoverOver = val => {
    this.setState({ selection: val })
  }

  setRating = val => {
    this.setState({ rating: val })
  }

  get rating() {
    return (
      (this.state.rating !== null ? this.state.rating : this.props.rating) || 0
    )
  }

  render() {
    return (
      <div onMouseOut={() => this.hoverOver(0)}>
        {oneToFiveArray.map(v => (
          <Star
            id={v}
            key={`star${v}`}
            isMarked={
              this.state.selection
                ? this.state.selection >= v
                : this.rating >= v
            }
            onHover={this.hoverOver}
            onRate={this.setRating}
          />
        ))}
      </div>
    )
  }
}

Also it needs to be accessible which it isn't right now

[Chalarangelo]

@atomiks I can move the component outside, I thought that if I inlined it, it would not pollute the global namespace, but it's actually slightly better with it outside the parent component. Meanwhile, if you check the updated version with onClick on the parent, star-id is used to identify each star uniquely and handle clicking on it, so I'm gonna skip that change.

props.rating should map to the state.rating not to something else - this is in no way implied that fetches data from a server, but rather works as an input that works for one user in my implementation. However, I see what you mean and I think it's not a bad idea at all.

As far as accessibility is concerned, please suggest something, I know that is an issue, I have no idea how to deal with it, though.

TL;DR: I'm gonna move the <Star> component outside and clean that up just a bit. The rest will probably need more changes before merging still, so fire away after I make those changes.

[Chalarangelo]

@atomiks The component is not inlined anymore. Could use some more pointers on how to improve this.

[atomiks]

I stand by what I wrote 🐝

Checked on CodeSandbox and both behave the exact same.

star-id is used to identify each star uniquely and handle clicking on it, so I'm gonna skip that change.

That's already done by its id though? (Gets passed as an argument to onRate)

[Chalarangelo]

@atmiks check the updated code, the setRating method (poor name after the changes, should be handleClick) is not bound for each <Star> but rather the parent component!

[atomiks]

Event delegation? Hm, any reason why?

[Chalarangelo]

@atomiks If you read skatcat31's comment, it seems like it's a bad idea to bind 5 onClick handlers for each <Star> when the whole thing acts as one input component. Imagine a scenario where you have hundreds of these components in a website, that would mess your browser up probably. Therefore, I went with that solution, which is slightly less messy performance-wise.

[atomiks]

In this case it makes it messier and non-React like for minor perf benefit. It's 5x fewer bindings - the main reason I'd use event delegation is when adding new children without registering new listeners or there are thousands of children which isn't a problem here.

Also you're still binding onMouseEnter to each star instead of using a single onMouseOver on the parent.

[Chalarangelo]

@atomiks I've found that onMouseOver prevented onMouseClick from firing for some weird reason on all of the <Star> components. onMouseEnter did not have that problem, so I went with that. If I somehow messed this up and onMouseOver works fine, it would be better to actually have a single handler on the parent for that and nothing else on the children.

PS: Is there a way to test React component's performance like jsPerf does for simple JS code? We could really use that.

[skatcat31]

@atomiks event delegation is preferable for variable amounts of children due to the fact that we aren't limited to a 5 star limit, but instead we are limited to however big we want based on the size of the array which could technically be done in the constructor as a numberOfStars prop or something so it's contained in the class in your proposal

@atomiks have we decided to use the class fields proposition? I thought we weren't using things that haven't been finalized or has it moved past stage 3? I know they're implementing it in tooling, but I was unaware of it's current status if it has been approved for ESN or if will be coming after as a stage 4. AFAIK it's still stage 3 but has had testing and an un-updated status since June in the list that was updated as of 7 days ago by ljharb.

@Chalarangelo you'll see performance degradation as each parsed function will delay the render, and it updates on mouse move(which could mean a per frame update, which means sub 16ms performance is required) and without de-bouncing could immediately impact performance with just 2 stars and heavy mouse movement. The solution would be no new bindings, or a de-bounced state update based on timeout to instance change with bound function. However a good middle ground is avoiding O(n+x) complexity and large values for n/other higher orders is to just put a warning and let the developer worry about their implementation. That may be rendered moot by how the key logic works for vdom in React however. This is after all an example, and my vote was just for a warning while moving the functional private component inside an IIFE.

const StarRating = (function(){
  function Star({ marked, starId, onHover }) {
    return (
      <span
        star-id={starId}
        style={{ color: '#ff9933' }}
        onMouseEnter={() => onHover(starId)}
      >
        {marked ? '\u2605' : '\u2606'}
      </span>
    );
  }

  return class StarRating extends React.Component {
    constructor(props) {
      super(props);
      this.state = {
        rating: typeof props.rating == 'number' ? props.rating : 0,
        selection: 0
      };
      this.hoverOver = this.hoverOver.bind(this);
      this.handleClick = this.handleClick.bind(this);
    }
    hoverOver(val) {
      this.setState(state => ({ selection: val }));
    }
    handleClick(event) {
      const val = event.target.getAttribute('star-id') || this.state.rating;
      this.setState(state => ({ rating: val }));
    }
    render() {
      return (
        <div onMouseOut={() => this.hoverOver(0)} onClick={this.handleClick}>
          {Array.from({ length: 5 }, (v, i) => i + 1).map(v => (
            <Star
              starId={v}
              key={`star_${ v } `}
              marked={
                this.state.selection
                  ? this.state.selection >= v
                  : this.state.rating >= v
              }
              onHover={this.hoverOver}
            />
          ))}
        </div>
      );
    }
  }
})()

Note:

This did not take into account @atomiks suggestions, it was a simple copy pasta to show that we don't want to leak multiple Star()s and possibly not allow them to be used by other components due to tight binding of logic.

[Chalarangelo]

@skatcat31 I see what you mean, I really really like this idea of using an IIFE, as it is the best middle ground.

As far as non-finalized things go, that's a hard no from me. Let's stick to things that work properly across the board. We can always come back later and change the code.

So what about handling all the movement without binding children's events to the handler?

[skatcat31]

That's what I'll be looking at when I'm done filling out an expenses report

[skatcat31]

Come to think of it, can we just add a hover style to element and less than n children?

[Chalarangelo]

@skatcat31 As a CSS person, I thought about this and it sounded like a good idea at first... Until I realized that we would be adding <style> tags from each rendered component and we would still have to process events to figure out which elements to mark that way or do some pretty ugly things in CSS for it to work.

[atomiks]

I'm not that good with accessibility but I know that:

• Clickable things should be buttons
• aria-label can help label things for screenreaders

Problem: It increases code considerably yet accessibility isn't something that should just be ignored! Maybe we need two snippets like "Basic" vs. "Fully accessible"? 😖

function Star({ id, isMarked, onHover, onRate }) {
  return (
    <button
      aria-label={`Rate ${id} out of ${TOTAL_STARS}`}
      style={{ color: "#ff9933" }}
      onClick={() => onRate(id)}
      onMouseEnter={() => onHover(id)}
    >
      {isMarked ? "\u2605" : "\u2606"}
    </button>
  );
}

class StarRating extends React.Component {
  state = {
    rating: null,
    selection: 0
  };

  hoverOver = val => {
    this.setState({ selection: val });
  };

  setRating = val => {
    this.setState({ rating: val });
  };

  get rating() {
    return (
      (this.state.rating !== null ? this.state.rating : this.props.rating) || 0
    );
  }

  get stars() {
    return [...Array(this.props.maxRating)].map((_, i) => i + 1)
  }

  render() {
    return (
      <div
        onMouseOut={() => this.hoverOver(0)}
        aria-label={
          this.state.rating
            ? `You rated this ${this.state.rating} stars`
            : this.props.rating
              ? `Rate this item. Current rating: ${this.props.rating} stars`
              : "Be the first to rate this item."
        }
      >
        {this.stars.map(v => (
          <Star
            id={v}
            key={`star${v}`}
            isMarked={
              this.state.selection
                ? this.state.selection >= v
                : this.rating >= v
            }
            onHover={this.hoverOver}
            onRate={this.setRating}
          />
        ))}
      </div>
    );
  }
}

StarRating.defaultProps = {
  maxRating: 5
};

I'm not sure if aria-label is read if the item isn't in focus btw. That means the parent <div> would need tabindex="0"

[Chalarangelo]

@atomiks Buttons have inherent styling across all browsers, so we would have to go out of our way to change their style to something different. We can just use role="button" and be on our merry way.

[Chalarangelo]

Updated the snippet with an IIFE as suggested, added role="button" for accessibility and move the onMouseOver event to the parent, so that we now have 3 events on the parent component and none on the children. This should now be almost up to standards I hope.

[skatcat31]

We still have one parsed function that can be changed to a bind. This can be dismissed since it's mostly a flavor thing, but it shows a better design pattern of using known situations to design consistent complexity

[skatcat31]

We could just use the i value directly

[Chalarangelo]

@skatcat31 Great suggestions, all changes applied, I think we're set!

[atomiks]

Errm there are still many problems with this 😒

• role="button" is not accessible because the stars can't receive focus
• Not using class properties proposal.. JSX is not even valid JS and the vast majority of React build tools come with this particular Babel plugin
• The IIFE is not necessary because of ES modules
• Using a function in setState even when not relying on the current state
• Still don't think the event delegation is useful in this case, binding event listeners is fast enough
• No aria-labels as I suggested?

We should have totalStars as a prop and use 5 as the default:

StarRating.defaultProps = {
  totalStars: 5
};

Also should use Squash & Merge because there are about 20+ "Updated" commits with no value now 😅

I suggest we revert the merge and incorporate those changes at the very least.

See: #12 (comment)

[Chalarangelo]

@atomiks I generally prefer that we don't squash when there are multiple authors. Let's work on the existing file right away. Open a PR and we can start from there.

[Chalarangelo]

Also class properties are Stage 3, we are not going to be doing that until it moves to Stage 4, in case we run into problems with it.

[atomiks]

Also class properties are Stage 3, we are not going to be doing that until it moves to Stage 4

I understand, but the majority of React projects are using it so they'd all face the same problem if the spec changed. So I don't see a problem with this project using it for now. It simplifies the code greatly. Also Stage 3 is pretty safe compared to Stage 2 and prior.

[Chalarangelo]

Still, we generally don't use Stage 3 features in other project and I don't think we should start now even if the majority of projects use this feature.

[skatcat31]

@atomiks majority does not mean adoption. You can in fact use React without Babel. Granted at that point using JSX becomes the oddity.

That aside, while you personally like them it seems, I am at odds with them as long as we have a single constructor in our language. If we had more than one constructor I would be ALL for the suggestion. However this is going off topic.

We would need to open an issue and decide now, as an org, if we should adopt that proposal in our React code base, if not the org as a whole at this moment in time(if they reach stage 4 and are planned for release, then I see no problem with them whatsoever)

[skatcat31]

That aside,

IIFE

The IIFE does seem a little confusing if you use ES Modules, however if you are just copy and pasting, we need a way to not overwrite any other Stars that other examples would create, which and IIFE avoids.

We could account for this in the build script, and when we do remove the IIFEs, however importing multiple files versus a single build would be a problem for slower internet users(slower initial load due to multiple tunnels), however would be a boon to updating a single script versus the entire collection. It also allows updates in piece wise. These will need to be thing to be considered when we do the build scripts.

role=button

a fair point, maybe we should change the stars to something that can receive focus or move to the aria-label

state not being used yet using a callback

we should update the one that does not derive from state to use a non callback, and the one that does should be nested correctly, good catch I missed that we were relying on state in one of them, and using the callback is harmless, but reads awkwardly

event listeners binding

If this were not such a heavy redraw possibility I would 100% agree. However if this gets adapted to more than 5 children, used in multiple places(most likely use case), etc. this snippet could easily cause issues. It would be better to address that now than later, and it was addressed by the submitter, so no skin off our back and at this point us changing it would be a slap in the face regardless of who submitted it.

total or max default prop

I agree we should move it to a prop. We should PR that. I do however remember us deciding not to use the Class.defaultProps property on a class due to the way it muddles code and linting for humans. See #1 and the comments on the issue that lead to a group decision of using language features to default the props. "Where did that magic number come from?" and such. If this were a generated repository then that tool would be excessively useful, but that is then up to the build tool to decide since we would not be reading that file, but instead the human code before transpiler.

squash and merge

This tool, while useful, is really for when you do an initial. Here that applies. IFF the squash keeps credit of all who helped with it then that would be fine as well with non initial code changes.

However often times when trying to address a bug you want that history because it lets you see when the actual problem came in and can often be a much simpler process to bisect and assign proper blame. Granted there may have been improvements to squash that still allow that. I have not checked the source code of Git in quite some time in regards to squash so I do not entirely know it's currently functionality.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for any follow-up tasks.