Repository navigation
Conversation
29568ef to
cb5d53c
Compare
drgrice1
left a comment
drgrice1
left a comment
There was a problem hiding this comment.
This looks good. Just a couple of suggestions.
Could you fix the help file for this page. There is an ending parenthesis with no starting parenthesis on line 22 in the help text.
| // Check or uncheck assignment checkboxes for versioned sets and their template to make it clear to the user what | ||
| // the backend will do with their selections. | ||
| document.querySelectorAll('input[type="checkbox"][name^="set."][name$=".assignment"]').forEach((checkbox) => { | ||
| allCheckboxes.forEach((checkbox) => { |
There was a problem hiding this comment.
This is a clean up step that I have been trying to handle as the javascript files are modified. I want to eliminate most forEach usage. The only case where this is really beneficial is for a one liner such as the instance on line 23 in this file. The other cases should use a for (const ... of ...) { ... } loop instead. This is something that many style guides recommend because it is more readable.
You don't have to do this, but if you could it would be nice, and it isn't hard to do! So here this would become
| allCheckboxes.forEach((checkbox) => { | |
| for (const checkbox of allCheckboxes) { |
and then remove the ); on line 27.
I would make the same change for the forEach calls added lines 45 and 51.
There was a problem hiding this comment.
What about line 45, should that be made a one liner or not (since you said it was an acceptable use)? I am guessing not because we are using the predefined allCheckboxes constant, and not appending that method to a query.
There was a problem hiding this comment.
Yeah, I was going to say that line 45 could also be made a one liner instead of converting to a for .. of loop.
cb5d53c to
decf4b9
Compare
Update the displayed information on the user detail page.
+ State how many sets a user is assigned to out of the total number.
+ Change the "edit class list data" link to a button.
+ Only show the "Assign All sets to Current User" button if the
user isn't assigned to all sets, and state the user's name
instead of "Current User".
+ Remove all the warnings from the page. Add a statement that
unchecking the "Assigned" checkbox will unassign a set and
cannot be undone.
+ Call the table "Available Sets", as it contains more sets
than the ones assigned to the user.
Add a confirmation modal that appears whenever a set is unchecked
to require confirmation. This also adds a hidden field so the
unassign on the server side will only unassign if that hidden
field had been updated to true.
Add a check to see if any of the dates were actually changed, and
only update the database entry in the case a change was made to
avoid unneeded database calls.
There was some logic which I believe was trying to allow a set date
to be "0", but it wasn't working, `param('x') && param('x') ne ''`
was being used, which I think was meant to allow "0", so the logic
should have been `defined param('x') && param('x') ne ''`, but
the method `checkDates` was already explicitly disallowing a "0"
date. Except for the `checkDates` method, the logic was all
simplified to just checking for the truth of `param('x')`, since
"0" was being disallowed in the `checkDates` check anyways.
Add a hidden save submit button first (if showing the assign all
button), so pressing enter in the form will be a save and not an
assign all action.
|
@drgrice1 Do we need to worry about escaping the username here? Or I assume all of Mojolicious helpers already do that? Just wanted to make sure the PRs that I've been using that print the user's first and last name didn't need additional escaping that was overlooked. |
decf4b9 to
26b6dba
Compare
|
Where specifically are you referring to for escaping the username? If it is inside a |
|
From what I can see here, all are in |
2 participants
Update the displayed information on the user detail page.
Add a confirmation modal that appears whenever a set is unchecked to require confirmation. This also adds a hidden field so the unassign on the server side will only unassign if that hidden field had been updated to true.
Add a check to see if any of the dates were actually changed, and only update the database entry in the case a change was made to avoid unneeded database calls.
There was some logic which I believe was trying to allow a set date to be "0", but it wasn't working,
param('x') && param('x') ne ''was being used, which I think was meant to allow "0", so the logic should have beendefined param('x') && param('x') ne '', but the methodcheckDateswas already explicitly disallowing a "0" date. Except for thecheckDatesmethod, the logic was all simplified to just checking for the truth ofparam('x'), since "0" was being disallowed in thecheckDatescheck anyways.Add a hidden save submit button first (if showing the assign all button), so pressing enter in the form will be a save and not an assign all action.