put aria-sort attributes on tables in Instructor Tools - #3094
Conversation
somiaj
left a comment
somiaj
left a comment
There was a problem hiding this comment.
Works as expected.
I noticed that the "Job Manager" table doesn't include aria-sort in its table if you wanted to fix this there too.
|
Good catch, I pushed the same changes for the Job Manager. |
| <% end =%> | ||
| </th> | ||
| <th> | ||
| <th <%== $c->ariaSortAttr('user_id') %>> |
There was a problem hiding this comment.
I am unsure what is better here, but attaching this to the $c object seems like what isn't done else where. Instead I would have done it like above. Include ariaSortAttr at the top of the template file:
% use WeBWorK::Utils::Instructor qw(ariaSortAttr);
Then call it as <%== ariaSortAttr($c, 'user_id') %>.
And remove the includes from the .pm files.
I don't know if one is preferred or not, but it isn't uncommon in template files to include and use the utilities directly vs attach it to the controller object.
There was a problem hiding this comment.
Everywhere else in this PR calls ariaSortAttr as a method on $c. Are you saying they should all be changed to ariaSortAttr($c, ... )? I couldn't tell if your comment is only about this one instance.
There was a problem hiding this comment.
Yea, I twas about not adding the method to the controller object and just including it directly in the template file it is needed, and update all cases that do that.
There was a problem hiding this comment.
Note to me this is more what is done elsewhere, and I just thought there was no reason to add this method to the controller object. But I'm unsure if there is any reason that one way might be preferred over another, it just originally looked odd to me compared to what I've seen done elsewhere.
There was a problem hiding this comment.
Is it worth leaving it this way simply to not have to import the subroutine at the top of these files?
There was a problem hiding this comment.
BTW, something went wrong with my IT team's OS patches to my development server. I can't really test things right now, so I'm not trying any code changes at the moment.
There was a problem hiding this comment.
I would probably see if @drgrice1 can clarify if such a change is worth it, or if it just a matter of taste.
There was a problem hiding this comment.
I would rather see the method imported in the file it is used. That makes it clear why the import is there. So I agree with @somiaj to import the ariaSortAttr in the template.
Unfortunately I only have my phone right now, and can't effectively look closely at the code to give a proper analysis though.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
The set list and user list tables now put
aria-sort="ascending"/"descending"on the<th>of the primary sort column. Andaria-sort="other"on any other (seondary or ternary) currently-sorted column's<th>, sincearia-sortcan only represent one column as strictly ascending/descending at a time.