improvements to Instructor Tools #2266
Conversation
|
I forgot something. Before these changes, the Instructor Tools page has a button to take you to the Email page, and another button to take you to the File Manager. I removed these. They seem unnecessary with those things being in the nav menu, and also having nothing to do with selecting users or sets. |
|
@Alex-Jordan would having a button 'Email selected users' be useful? They can do this directly from the email page, so not really needed but could be an alternative to just removing it. Note, I think removing it is fine, this is just a though that passed my mind. |
This uses `flex` rules to make the category input groups behave better. The categories are wrapped in a div with fixed width. The width is chosen to be as small as possible such that the input groups do not wrap. Thus this dispenses with trying to make the category widths match the scrolling record list which really wasn't working. `flex-wrap` is used with "around" justification. So at extra large window widths all three categories will be in the same row. However, when the categories don't fit, they wrap. The justification gives nicer positioning in any case. I merged the input texts, removed capitalization, and changed some of the wording in a few cases. For example, I don't think that the "Library Browser Exercises" is good. I think the reference to the library browser is unnecessary, and using "exercises" instead of "problems" is inconsistent the wording used everywhere else in webwork2. The other changes made are more explicit in the comments made in my review.
There was a problem hiding this comment.
Line 26 of templates/HelpFiles/Hardcopy.html.ep and line 24 of templates/HelpFiles/InstructorAssigner.html.ep need to be changed. The "Change Display Settings" buttons on those pages are now the "Refresh List" buttons.
I think that the button on the instructor "Email" page should also be changed to be a "Refresh List" button for consistency. That is set on line 134 of templates/ContentGenerator/Instructor/SendMail/main_form.html.ep. Probably just delete that line and use the default so that it will be in sync with the other places that the scrolling record list is used. Also make sure to change line 22 of templates/HelpFiles/InstructorSendMail.html.ep.
I think that the restructuring of the input groups and the three categories looks pretty bad. Trying to fit all three onto one row just really doesn't work. At almost any window width, the input groups wrap. Input groups do not wrap well at all. The border radii are incorrect when wrapping occurs. Things also look unbalanced above the xl breakpoint because of the different sizing of the categories and buttons.
I also don't like the way that the input texts are split into parts. That problematic for translations (it already was some but now it is worse). I think this was done to mitigate wrapping of input groups since the last input text grows, but that still doesn't really help because a wrapped input group just looks bad due to the border radius not working out when wrapping occurs.
The upper/lower case mixing also doesn't look very nice. I think you are trying to associate the action with the menu link names, but I don't think the capitalization is needed for that.
I have added pull request Alex-Jordan#20 to this branch with my suggested way to deal with the issues noted in this review.
This uses `flex` rules to make the category input groups behave better. The categories are wrapped in a div with fixed width. The width is chosen to be as small as possible such that the input groups do not wrap. Thus this dispenses with trying to make the category widths match the scrolling record list which really wasn't working. `flex-wrap` is used with "around" justification. So at extra large window widths all three categories will be in the same row. However, when the categories don't fit, they wrap. The justification gives nicer positioning in any case. I merged the input texts, removed capitalization, and changed some of the wording in a few cases. For example, I don't think that the "Library Browser Exercises" is good. I think the reference to the library browser is unnecessary, and using "exercises" instead of "problems" is inconsistent the wording used everywhere else in webwork2. The other changes made are more explicit in the comments made in my review.
|
I agree that the "Email" and "File Manager" buttons are not needed by the way. |
Implement suggestions made in my review of openwebwork#2266
|
Thanks @drgrice1 for the review and improvements. More tools could be added. I'm polling to see what we would like.
And as mentioned at #1151, I'd like to add "Randomize seeds for all selected assignments for all selected users". But that can come later when that feature is a thing in the first place. |
|
I added the three tools mentioned in my previous comment. The text description for those tools was long and I needed to grow the 350px size to 370px. This approach has fragility in at least two ways. A user might rescale font size and then these lines are still not long enough. Or a localization might be longer. Since the There was an adjacent I had to change the name of the user select element in the Email page to make it match the name of the user select element from the Instructor Tools page. There was one place in |
|
Switch to using the |
|
That is with the font scaling. Not the translation issue. |
|
Another option that will generally resolve most issues is to just use a large enough width to leave excess. That won't entirely resolve the issue, but probably nothing will. |
|
I did a little of that. 27em was enough, but I took it to 28. |
|
It seems that |
|
The size seems fine for me, I don't see the wrapping for "View answers for selected users, for selected sets" here. Could this one be shortened? Maybe "View selected users' answers for selected sets". Also "Score selected sets for ALL users", ALL is not fully valid here, maybe "Score selected sets for active users" or "score selected sets for active students"? |
I think that "all" is fine here, and I think is better than "active". "active" can mean many things. Actively logged into the course. Actively working on the assignment in question. Furthermore, when a set is scored, all users that the set is assigned to are scored, even if they have not "actively" worked the set. |
|
Actually, testing further, it really is ALL users. Even those that are not assigned the set are scored. |
|
Okay, so dropped students are not scored, but there is no reason to really be that detailed here. |
|
There is another sizing aspect to fiddle with. The buttons could possibly be made a little smaller. Currently we are using a width of 25%. That could be reduced to 20%. Unfortunately, bootstrap doesn't have a class for that, so it would have to be done with an inline style attribute. Of course, if a translation of one of the button labels is longer there could be problems with that. |
|
I suppose you could also go back to the different sizing of the three categories that you had in this pull request to start with. Make the second category with all of the longer phrases a bit bigger, and the other two smaller. |
|
Since "Score" is not dependent on any user selections, I could move it to the left column. Is that a good move? I should add at least one user format option that makes use of permission level and role. Then you would be able to sort by those too. And I could add filtering by those things as well. The 28 em (and I believe 27em) is working for me with FF, Chrome, Safari. I wonder if it's something about me (and I assume @somiaj ) using a Mac. Anyway I will make it bigger the next round of edits. BTW I was thinking to make that a setting in some css file, but if we really are adjusting it to accommodate certain string lengths, it seems appropriate to set the width inline. We could use We could move to a tabbed menu for the three categories to make the spacing issues moot. At the cost of not seeing all the tools right away. |
Although all of the left column's actions involve only users. Also, then the left column would have more than all of the others, making it unbalanced.
That would be nice to have. That would take some rewriting of the scrolling record list though since the permission level is not in the same database table.
I also thought that a css file might be needed. Although, we don't have that many inline styles here (yet).
I think a different approach like that is what is going to be needed in the end. This approaches of trying to fit the three categories in at either a fixed width or a variable width using the bootstrap grid layout are just not entirely working. |
|
Oh I meant whichever side has the set selection, so the right side. I got
mixed up. With that move plus eventually adding "randomize" to the middle,
I think the three columns end up with equal size.
…On Wed, Nov 29, 2023, 7:44 PM Glenn Rice ***@***.***> wrote:
Since "Score" is not dependent on any user selections, I could move it to
the left column. Is that a good move?
Although all of the left column's actions involve only users. Also, then
the left column would have more than all of the others, making it
unbalanced.
I should add at least one user format option that makes use of permission
level and role. Then you would be able to sort by those too. And I could
add filtering by those things as well.
That would be nice to have. That would take some rewriting of the
scrolling record list though since the permission level is not in the same
database table.
The 28 em (and I believe 27em) is working for me with FF, Chrome, Safari.
I wonder if it's something about me (and I assume @somiaj
<https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-a2dbbe6078f1ac06&q=1&e=c50cfcc8-aaa5-4622-af2b-12b8b2c2a383&u=https%3A%2F%2Fgithub.com%2Fsomiaj>
) using a Mac. Anyway I will make it bigger the next round of edits. BTW I
was thinking to make that a setting in some css file, but if we really are
adjusting it to accommodate certain string lengths, it seems appropriate to
set the width inline.
I also thought that a css file might be needed. Although, we don't have
that many inline styles here (yet).
We could move to a tabbed menu for the three categories to make the
spacing issues moot. At the cost of not seeing all the tools right away.
I think a different approach like that is what is going to be needed in
the end. This approaches of trying to fit the three categories in at either
a fixed width or a variable width using the bootstrap grid layout are just
not entirely working.
—
Reply to this email directly, view it on GitHub
<https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-019234f71c6b2c39&q=1&e=c50cfcc8-aaa5-4622-af2b-12b8b2c2a383&u=https%3A%2F%2Fgithub.com%2Fopenwebwork%2Fwebwork2%2Fpull%2F2266%23issuecomment-1833060208>,
or unsubscribe
<https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-f0b3387a4ef73567&q=1&e=c50cfcc8-aaa5-4622-af2b-12b8b2c2a383&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOADH6YQMXMDP3SHPGTDYG76IBAVCNFSM6AAAAAA75EW5B6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZTGA3DAMRQHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
That makes more sense. That is a good move then. |
In I could change it to "Score selected sets for scorable users"? |
|
If I go to 29em, then even at full screen width on a MacBookPro, if the nav toc is there, the three columns do not fit on one row. Anyway I will explore a tab setup for this now. |
How about just "Score selected set" and don't say anything about users just like it does on the scoring page itself? |
|
Here is another possible layout suggestion:
|
lib/WeBWorK/Utils/FilterRecords.pm
Outdated
| @@ -117,7 +161,7 @@ C<$filters> should be a reference to an array of filters or be undefined. | |||
| =cut | |||
|
|
|||
| sub filterRecords { | |||
| my ($filters, @records) = @_; | |||
| my ($c, $filter_combine, $filters, @records) = @_; | |||
There was a problem hiding this comment.
The $filter_combine parameter really shouldn't be a string value. It should be a boolean since it is either union or intersect (an on/off state). Basically, just use $intersect for the parameter, and delete line 183.
|
Instead of trying to squeeze the intersection and union radio buttons to the left, where it is really never going to fit, just put it in a horizontal format below the filter select (sort of as @somiaj suggested, but leave the filter label to the left as it is). |
|
Also, you have the problem that came up before with the current structure of the grid layout also. There are elements with |
Switch the new `filterRecords` parameter to a boolean, and adjust the radio values to make that easier to work with. Place the radio buttons and labels in a horizontal layout below the filter select.
|
I added another pull request that implements the suggestions I made in the last few comments. |
More suggestions.
|
There is now one little cosmetic thing that @somiaj found. If you make the screen narrow so that the main content is a little over 500px wide, you don't have space for some words: "Sort By:", "Format:", and "Filter(s):". |
Yeah, that was that way before though. Although before it wasn't quite as bad, and only really affected "Format". This only happens right at the cut of from the Also, I think that @somiaj is right that comments in the help files might be a good idea for the union/intersect radios. Unfortunately, that will need to be added to all of the pages that use the scrolling record list. Or perhaps you could create a separate template help file for scrolling record lists, and include that in the help files that use it. |
|
One option would be to drop the horizontal layout when you are below the |
|
I added a pull request that drops out of the horizontal layout below the |
|
It is to bad that there is not a css media query that applies to container width instead of window width, because things are quite different in the range from the |
for the scrolling record list.
|
I further tweaked the pull request so it only changes between the |
|
There is something else that I missed with the union/intersect radio button labels. The font sizes for those are a bit larger than the other labels. This is because the others use the col-form-label-sm class. There is not |
|
I added the |
in size. The "Users" scrolling select is now a bit longer due to the addition of the intersect/union radio buttons.
Below the `lg` breakpoint, drop out of the horizontal layout for the scrolling record list.
I added these. I did it in each place. The scrolling record list is not used consistently (sometimes it's only used for users, sometimes it is used for users and sets) and because of that it seems like it could get awkward to word things well in a uniform way in some common template. |
|
OK, unless I forgot about something, I think this is ready for last looks. |
Fixed now. Searching for |
Alex-Jordan
force-pushed
the
instructor-tools
branch
from
f5b0564 to
07b04fd
Compare
|
Yeah, I try to do that on occasion. I want to keep the template code clean, because there is nothing like perltidy for those files to help with formatting the template code. |
This makes changes to Instructor Tools. Also it makes changes to
scrollingRecordListwhich is used by Instructor Tools, and also by the Assigner Tool, Email, Hardcopy, and Past Answer pages. So testing should look at all five pages.There are cosmetic changes that could be better. I won't describe them here; feel free to make suggestions for improving anything cosmetic. The functional improvements are:
formatDateTimeand presently generates an error message. But Improvements to date/time string handling. #2263 will make that go away.A screenshot of what to expect: