Update display of question settings page #9687
Conversation
All images
|
| } | ||
| </div> | ||
| </div> | ||
| </form> |
There was a problem hiding this comment.
I ended the form here. This is to prevent nested forms as we already have forms for public sharing and tests already on the page. However, I believe all of the fields that will want to be editable are captured in this form. That being said, is there a better way to do this? My concern here being that once we are ready to add a submit button, I am not sure where the best place is going to be for that.
There was a problem hiding this comment.
I think this makes sense; we'd probably want to put the "save" button below the top part of the page. Of course, we're sort of developing new patterns here, so we can adjust as we go!
apps/prairielearn/src/pages/instructorQuestionSettings/instructorQuestionSettings.html.ts
Outdated
Show resolved
Hide resolved
| ${ | ||
| resLocals.authz_data.has_course_permission_view | ||
| ? resLocals.authz_data.has_course_permission_edit && | ||
| !resLocals.course.example_course | ||
| ? html` | ||
| <a | ||
| href="${resLocals.urlPrefix}/question/${resLocals.question | ||
| .id}/file_edit/${infoPath}" | ||
| > | ||
| Edit question configuration | ||
| </a> | ||
| in <code>info.json</code> | ||
| ` | ||
| : html` | ||
| <a | ||
| href="${resLocals.urlPrefix}/question/${resLocals.question | ||
| .id}/file_view/${infoPath}" | ||
| > | ||
| View course configuration | ||
| </a> | ||
| in <code>info.json</code> | ||
| ` | ||
| : '' | ||
| } |
There was a problem hiding this comment.
This piece was intended to be similar to the course settings page.
apps/prairielearn/src/pages/instructorQuestionSettings/instructorQuestionSettings.html.ts
Outdated
Show resolved
Hide resolved
| <div class="form-group"> | ||
| <div>Topic</div> | ||
| <div name="topic"> | ||
| ${renderEjs(__filename, "<%- include('../partials/topic') %>", { | ||
| topic: resLocals.topic, | ||
| })} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
I don't think the form-group class will do much to help us here, and setting name="..." on a div won't do anything at all.
For better accessibility, I'd recommend using heading tags (<h1>, <h2>, etc.) as appropriate for these sections. Here's a primer if you're new to those: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements
There was a problem hiding this comment.
Updated these to use <h2> for these sections in f9945bf. The display of <h2> was quite large for these subheaders so added class="h5" to reduce the text to a more appropriate size.
| ${renderEjs(__filename, "<%- include('../partials/tags') %>", { | ||
| tags: resLocals.tags, | ||
| })} |
There was a problem hiding this comment.
Now that we're giving ourselves more vertical space, maybe we render this as a list group instead? Something like this:
Though we'd want to align the name/description in different columns, sort of like GitHub's "labels" page:
Bootstrap's grid system will probably come in handy for that.
There was a problem hiding this comment.
In f9945bf, updated the "Tags" section to look like the following: 
| <div class="form-group"> | ||
| <div> | ||
| Issues | ||
| ${renderEjs(__filename, "<%- include('../partials/issueBadge') %>", { | ||
| count: resLocals.open_issue_count, | ||
| issueQid: resLocals.question.qid, | ||
| suppressLink: resLocals.suppressLink, | ||
| urlPrefix: resLocals.urlPrefix, | ||
| })} | ||
| </div> | ||
| </div> | ||
| <hr /> | ||
| <div class="form-group"> | ||
| <div>Assessments</div> |
There was a problem hiding this comment.
This bit looks funky when there aren't any issues/assessments:
A few ideas:
-
We might consider actually adding an "issues" tab to the instructor question view? That way we could get this out of this page entirely (it's a bit weird to have a link to a list of issues on a "settings" page IMO.
-
We could show the list of assessments with a Bootstrap list group, and maybe show extra information, e.g. the course instance. We could then have a nice empty state when there aren't any, e.g.:
There was a problem hiding this comment.
Added an "issues" tab in f9945bf: 
Currently, this new tab just links to the instructorIssues page with a query param of qid%3A{question.qid}. This will take the user to the existing issues page and filter by the current qid. I can build a specific questionIssues page in a future PR if this is a bit funky, but that seemed like a large endeavor to include in this PR and this seems to provide the functionality we would want here.
The assessments section now looks like this:
Assessments are grouped by course instance and displayed in a card for readability. I'm not certain on the columns we want included though. Currently I have label/badge, type, and title. Let me know if you would prefer any other columns here or to remove the type column if that is redundant (or if I should order these differently?).
Also, questions that are not included in any assessments now show the following:
| </div> | ||
| </form> |
There was a problem hiding this comment.
The fact that these both end up at the same indentation level means that there's probably a mismatched tag somewhere above. See if you can find it; if not, I can take a look myself!
There was a problem hiding this comment.
Good catch, my <form> and subsequent <div> were backwards. Updated in f9945bf.
| <div class="card-body"> | ||
| <form> | ||
| <div class="form-group"> | ||
| <label for="title" class="h5">Title</label> |
There was a problem hiding this comment.
I would be inclined to use normal label styling here (not h5). If you want to label this top section as a whole to match the headings on the other sections, you could add a <h2 class="h5">General</h2> above this.
Note also that we (I, technically) have sorta/kinda been standardizing on <h2 class="h4"> on the administrator pages. Ultimately we can go with what we think looks best here, but can we try out class="h4" to see how it looks?
There was a problem hiding this comment.
Added a "General" section to the top and changed to `class="h4" in 381d728. How does this look?
| ? html` | ||
| <div> | ||
| <h2 class="h5">Tests</h5> | ||
| <div class="pl-3"> |
There was a problem hiding this comment.
Why add horizontal padding here?
There was a problem hiding this comment.
I had added horizontal padding here and elsewhere to help indent the information and help with readability. Removed in 381d728.
| <div> | ||
| <h2 class="h5">Tests</h5> | ||
| <div class="pl-3"> | ||
| ${questionTestsForm({ |
There was a problem hiding this comment.
Let's use PascalCase here.
There was a problem hiding this comment.
Updated to PascalCase in 381d728.
| }) { | ||
| if (questionSharedPublicly) { | ||
| return html` | ||
| <div class="row px-3"> |
There was a problem hiding this comment.
Can you say more about why you added horizontal padding here and elsewhere?
There was a problem hiding this comment.
As stated above, I thought it improved readability by indenting and adding a little bit of lateral separation from the header. Removed in 381d728.
| <div class="row px-3"> | ||
| <div class="col-1"> | ||
| ${renderEjs(__filename, "<%- include('../partials/topic') %>", { | ||
| topic: resLocals.topic, | ||
| })} | ||
| </div> | ||
| <div class="col-auto">${resLocals.topic.description}</div> | ||
| </div> |
There was a problem hiding this comment.
How about we wrap this in a list group? I know there will only ever be one topic, but it feels like the badge + description want to be contained in something, IMO. We could do a similar treatment for tags if you like this?
| <div class="row px-3"> | |
| <div class="col-1"> | |
| ${renderEjs(__filename, "<%- include('../partials/topic') %>", { | |
| topic: resLocals.topic, | |
| })} | |
| </div> | |
| <div class="col-auto">${resLocals.topic.description}</div> | |
| </div> | |
| <div class="list-group"> | |
| <div class="list-group-item d-flex align-items-center"> | |
| ${renderEjs(__filename, "<%- include('../partials/topic') %>", { | |
| topic: resLocals.topic, | |
| })} | |
| <span class="ml-3">${resLocals.topic.description}</span> | |
| </div> | |
| </div> |
There was a problem hiding this comment.
Updated to this here and for tags locally in 381d728.
| }); | ||
| } | ||
|
|
||
| function AssessmentRows({ assessmentsWithQuestion }) { |
There was a problem hiding this comment.
Some weird stuff happens on narrow viewports:
I'm honestly wondering if we need to show this here; it seems strange to show this on a "settings" page? One can already see all assessments on which a question is used on the "statistics" question tab. cc @mwest1066 @mfsilva22 for input: would you be sad if the assessments list on the question settings page went away?
There was a problem hiding this comment.
It wouldn't be terrible to remove it, but I think it makes sense to have it here. I don't think that we need to show titles for the assessments, if that's what's causing the problems? At the moment we just show a nice compact list like this:
There was a problem hiding this comment.
I updated to show the compact list by course instance in 381d728. Would something like this make sense?
and on narrower viewports:
We could also decide which content is hidden or displayed on certain size viewports by using Bootstraps built-in breakpoints (https://getbootstrap.com/docs/4.0/utilities/display/). This could also be useful for deciding if we render the descriptions in the topic and tags sections as well.
| </div> | ||
| ` | ||
| : ''} | ||
| <hr /> |
There was a problem hiding this comment.
I'd advise moving this inside the following conditional. As written, we end up with an extra horizontal line for questions that can't be tested.
There was a problem hiding this comment.
Moved the horizontal line into the conditional in 381d728.
| @@ -21,4 +21,9 @@ | |||
| urlSuffix: '/question/'+question.id+'/statistics', | |||
| iconClasses: 'fas fa-chart-bar', | |||
| tabLabel: 'Statistics'}) %> | |||
| <%- include('navTab', { | |||
| activeSubPage: 'issues', | |||
| urlSuffix: '/course_admin/issues?q=qid%3A'+question.qid, | |||
There was a problem hiding this comment.
I think we need to take care to properly URL-encode the qid here.
What do you think about possibly showing a badge here with an issue count? That should already be available in resLocals.open_issue_count IIRC. Something like how GitHub displays issue/PR counts:
There was a problem hiding this comment.
Yes please, a red badge like we already use for the main "Issues" navbar item.
There was a problem hiding this comment.
Thanks to @nwalters512 help on Zoom, added a red badge in 381d728.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9687 +/- ##
==========================================
+ Coverage 66.44% 66.79% +0.34%
==========================================
Files 453 454 +1
Lines 70319 71146 +827
Branches 5656 5689 +33
==========================================
+ Hits 46723 47519 +796
- Misses 23173 23200 +27
- Partials 423 427 +4 ☔ View full report in Codecov by Sentry. |
This PR is intended to be the first step in adding editing functionality to the question settings page. Much of the art for this was from the course settings page where this has been previously implemented. In this PR, we are moving the fields into a
<form>and updating the display to match our other editable pages. Future PRs will implement editing using theeditorclass.Here is an example of the first pass at this:
