Mini Rubric: Fix comment tab resize and re-implement keep feedback when switch tabs #27548
Conversation
… Breaks auto re-size of feedback tab area
…feedback-tab-no-showing
|
Open question @madelynkasula and @davidsbailey : Should I have a ui test that tests both the experiment off and experiment on? Right now it only tests with the experiment on. |
| @@ -315,7 +321,10 @@ class TopInstructions extends Component { | |||
| this.state.feedbacks.length > 0 && | |||
| (this.state.feedbacks[0].comment || this.state.feedbacks[0].performance); | |||
| const displayFeedbackTeacher = | |||
| this.props.viewAs === ViewType.Teacher && this.state.rubric; | |||
| this.state.displayFeedbackTeacherFacing || | |||
| (!this.state.displayFeedbackTeacherFacing && | |||
There was a problem hiding this comment.
why would this.state.displayFeedbackTeacherFacing be false here (i.e., for teachers in the experiment)?
There was a problem hiding this comment.
so we show the feedback tab for teachers in the experiment in two cases:
- viewing student work
- viewing level normally as a teacher(not viewing student work) and there is a rubric
So the biggest reason it is false is if you are not viewing student work and there is no rubric
There was a problem hiding this comment.
okay, that makes sense. i think some of this complexity will go away once there is no longer an experiment to deal with, but the display logic is getting pretty complex here. we should revisit when we're removing the experiment, or it may just fix itself 😄
|
@dmcavoy i think it would be useful to have a UI test and covers the case where the experiment is off, even just a simple test that checks 1) does the tab display and 2) does the content display |
This is really a judgment call -- because the non-experiment version will not live forever, I'd lean toward not adding one and just manually test the non-experiment version whenever complex changes are made. but if that sounds scary then adding a simple UI test is totally fine too. |
| @@ -153,6 +153,10 @@ class TeacherFeedback extends Component { | |||
| } | |||
| }; | |||
|
|
|||
| componentWillUnmount() { | |||
| window.removeEventListener('beforeunload'); | |||
| } | |||
There was a problem hiding this comment.
with this change, will there be a situation where our 'beforeunload' handler still gets triggered?
I'd expect this event to be added while there are pending changes and then removed when the changes are saved to the server. However, I don't have a lot of direct experience with handling this event so rather than prescribing a solution I'll just say please make sure you're getting these warnings at the times that you'd expect to.
There was a problem hiding this comment.
It still is triggering the warning when we would want it to.
I did this in response to the react documentation saying: "This method (componentDidMount) is a good place to set up any subscriptions. If you do that, don’t forget to unsubscribe in componentWillUnmount()."
Oops, sorry to contradict you here Maddie. It really is a judgment call and given that we've already experienced a regression, perhaps adding a UI test is best. |
1. Fixes bug where the feedback tab was not showing (but the feedback tab content was) for teachers who are not in the experiment.
Issue was that the boolean that determines when to show this tab for teachers contained statement about a rubric being present for that level.
Before
After
2. Re-implements #27507 which keeps the feedback that is unsaved when you switch tabs.
This had to be reverted because it used the boolean from issue 1 to determine when to show the feedback tab. Using the boolean works fine when that boolean is correct for non-experiment teachers
Before
After
No Experiment
Experiment
3. Fixes bug where you could not re-size the feedback tab contents because of the way that re-sizing the feedback tab on load to show all the content was implemented.
This was a result of the fact that we were always forcing the feedback tab to be full size instead of forcing it to be full size on load and then letting the user re-size as wanted.
Before
After
No Experiment
Experiment