Teacher Feedback: Don't load tab until we have all the content from network

[dmcavoy]

There was a bug in an eyes test that drove this investigation which led to some larger refactoring of the network requests for TeacherFeedback content.

Background

The bug in the eyes test was that when the student was viewing feedback from a teacher with a rubric the rubric would be expanded by default (which is not supposed to happen). We tracked this down to being the result of when we initially rendered the TeacherFeedback tab in this instance the network hadn't gotten back the student's feedback data so it would initially render as a Key Concept tab and then re-render to the Feedback tab but never reset the state which controls if the details tag start open.

Fix

Dave helped me work to determine that the best way to fix this was probably to wait on rendering the TeacherFeedback component until we have all the network requests in TopInstructions. I updated this but then felt like we should also move the network requests out of TeacherFeedback (specifically the one where it gets the feedback when a Teacher is viewing a student's work) so that in all cases we are waiting to render until we have the data.

This does fix the initial bug from above (see below):

And I tested that all the cases in and out of experiment remain consistent.

[davidsbailey]

Looks great, Dani! Consolidating the networks requests was a great call. I know it seems like a lot of work for a little tooltip... but in the bigger picture I think this is a good example of when a small change tips the scales and makes it time to do a larger refactoring that will have the added benefit of making other future changes easier too. Thank you for your perseverance in working toward a great solution on this!

[davidsbailey]

nice lift! it is always great to see these network requests getting moved farther up the rendering chain. and since moving them all the way up and out of react sounds like a headache, consolidating them into this one location seems like an excellent alternative.

[maddiedierker]

mapping a component's props to state like this is usually an anti-pattern in react, but, after reading this blog post, it seems okay to do in this context.

i usually get around this by moving the handlers that change state for these props into the parent like this:

class Parent extends React.Component {
  state = {
    comment: ''
  }

  onChangeComment = (event) => {
    this.setState({comment: event.target.value});
  }

  render() {
    return <Child comment={this.state.comment} onChangeComment={this.onChangeComment} />;
  }
}

class Child extends React.Component {
  render() {
    return (
      <input value={this.props.comment} onChange={this.props.onChangeComment} />
    );
  }
}

then, the parent component fully controls the state and the child is basically just rendering and calling methods that control state on the parent