Suggested clean up and minor issue fixes.#36
Merged
Conversation
The JavaScript needs to not attempt to do anything with the
`add_on_conf` select if it is not present in the page, as that causes
errors. For instance if the `$webworkDirs{addOnConf}` directory is not
present, or is empty.
With modern JavaScript, don't use the array `forEach` method unless it
gives a convenient and brief one liner. Generally prefer a `for of`
loop as it is more readable. Another advantage is that `for of` loops
work on `HTMLCollection`s and `forEach` does not. The result of a
`querySelectorAll` call or the `children` of an `Element` are
`HTMLCollection`s, and so a `for of` loop works directly, while
`Array.from` is needed to use `forEach`. This is a minor inefficiency in
this case, but that does require constructing an array from the
`HTMLCollection`.
I rewrote the `writeCourseConf` POD to clarify its usage some with the
new optional parameter.
In the `writeCourseConf` method, it should be checked that the
`$addOnConf` variable is an array reference, and not that it is not
equal to the empty string. Any numeric value or non empty string is not
equal to the empty string and would cause an error if `$addOnConf` is
set to any of those. The check should be that `ref $addOnConf eq 'ARRAY'`.
That is the only condition that guarantees an error will not occur and
that the value of the variable will work inside the conditional block.
Minor cleanup in the `templates/ContentGenerator/CourseAdmin/add_course_form.html.ep`
file. There is no need to copy the value of `$ce->{webworkDirs}{addOnConf}`
to a local variable. Particularly since that can be used directly even
in string interpolation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The JavaScript needs to not attempt to do anything with the
add_on_confselect if it is not present in the page, as that causes errors. For instance if the$webworkDirs{addOnConf}directory is not present, or is empty.With modern JavaScript, don't use the array
forEachmethod unless it gives a convenient and brief one liner. Generally prefer afor ofloop as it is more readable. Another advantage is thatfor ofloops work onHTMLCollections andforEachdoes not. The result of aquerySelectorAllcall or thechildrenof anElementareHTMLCollections, and so afor ofloop works directly, whileArray.fromis needed to useforEach. This is a minor inefficiency in this case, but that does require constructing an array from theHTMLCollection.I rewrote the
writeCourseConfPOD to clarify its usage some with the new optional parameter.In the
writeCourseConfmethod, it should be checked that the$addOnConfvariable is an array reference, and not that it is not equal to the empty string. Any numeric value or non empty string is not equal to the empty string and would cause an error if$addOnConfis set to any of those. The check should be thatref $addOnConf eq 'ARRAY'. That is the only condition that guarantees an error will not occur and that the value of the variable will work inside the conditional block.Minor cleanup in the
templates/ContentGenerator/CourseAdmin/add_course_form.html.epfile. There is no need to copy the value of$ce->{webworkDirs}{addOnConf}to a local variable. Particularly since that can be used directly even in string interpolation.