Rename lesson summarize methods #37121
Conversation
davidsbailey
force-pushed
the
rename-lesson-summarize
branch
from
ee5c6c3
to
f1a048c
Compare
| @@ -329,6 +329,7 @@ class ActivitySectionCard extends Component { | |||
| targetActivitySectionPos, | |||
| activityPosition | |||
| } = this.props; | |||
| const activitySectionText = activitySection.text || ''; | |||
There was a problem hiding this comment.
Fixes a misc bug I ran across while trying to test this manually. the split fails later if the text field is null, which is what happens if you create a new activity section without entering any text/description.
There was a problem hiding this comment.
I think in some of the other cases that I ran into this I fixed it in the script/edit.js or lesson/edit.js file. Seeing this I'm wondering it those changes should be moved? Probably would be good to standardize. Do you have thoughts on which one to use?
There was a problem hiding this comment.
Good call Dani! Trying to keep these sanitation steps all together makes sense. In general I'm in favor of doing it early, before putting it into redux, so that we don't have to remember to do it every time we read from redux. If you move this to the edit.js file, then one bonus is you can mark this prop as required, which will help highlight if we somehow miss a case, e.g. we might replace null with '' when loading from the server, but if the user took an action that led us into a bad state then we'd get a handy js warning.
I'll go ahead and move this to edit.js.
Follow-up from #36935 (comment)
Testing story
I confirmed that existing test coverage is hitting these summarize codepaths. I also manually verified that the lesson edit page still works for editing activities and activity sections.
Reviewer Checklist: