Progress: Course selection bug followup #22289
Conversation
Erin007
changed the base branch from
multi-script-course-selection
to
course-selection-bug
There was a problem hiding this comment.
LGTM after addressing comments (though not necessary to do the extractions now). apologies that some of these comments relate to stuff from previous PRs and the extraction comments even refer to some code that I initially added 😳 it just seems like that section is getting big enough that it's time to clean it up a bit.
| @@ -204,9 +202,11 @@ export default function sectionProgress(state=initialState, action) { | |||
| if (action.type === SET_VALID_SCRIPTS) { | |||
|
|
|||
| let validScripts = action.validScripts; | |||
| // Set defaultScript to Express Course to use if there are no validScripts | |||
| let defaultScript = action.validScripts.filter(script => script.name === "Express Course")[0]; | |||
There was a problem hiding this comment.
style nits:
let validScripts-->const validScripts(on previous line).filter(...)[0]-->.find(...)let defaultScript-->const defaultScriptaction.validScripts-->validScriptsvar-->let(line 227)
There was a problem hiding this comment.
+1 on dave's comment. we end up using "Express Course" in a few places now, so i would also pull this out into a constant called DEFAULT_SCRIPT_NAME (or something along those lines)
| // Use the default script (currently Express Course) so we don't have | ||
| // an empty dropdown. | ||
| scriptId = defaultScript.id; | ||
| validScripts.push(defaultScript); |
There was a problem hiding this comment.
this is starting to get a bit confusing, because now the part of the function that determines the scriptId is also modifying validScripts. As a first step, I would recommend moving the logic for if (validScripts.length === 0) {validScripts.push(defaultScript)} to line 226 and replacing the default and last case statement here with just
default:
scriptId = validScripts[0].idand also adding a comment explaining that one section computes validScripts and the other computes scriptId.
Ideally you would extract 1 or 2 self-documenting functions like this:
if (action.type === SET_VALID_SCRIPTS) {
const validScripts = computeValidScripts(...)
const scriptId = getDefaultScriptId(...)
}but given the time constraints it seems ok not to do that now.
| @@ -50,7 +50,6 @@ export default class ScriptSelector extends Component { | |||
| onChange={event => onChange(parseInt(event.target.value))} | |||
| style={styles.dropdown} | |||
| > | |||
| <option key="default" value={''}/> | |||
There was a problem hiding this comment.
removed this because clicking on the empty string option would cause all kinds of problems because it didn't set a scriptId, and the default script is now "Express Course"
There was a problem hiding this comment.
Thanks for doing this @Erin007 ! FYI @poorvasingal that there should no longer be an empty option in the progress tab's script selector.
Follow up to #22248. Change default script from Accelerated to Express, update tests to match and only show the default script in the dropdown if the dropdown would otherwise be empty.