Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to sections in ExamPage #12182

Conversation

nucleogenesis
Copy link
Member

@nucleogenesis nucleogenesis commented May 17, 2024

Summary

As discussed with @rtibbles this is one part of a greater set of updates with regards to how we present the new "sections" to users. This updates the quiz-taking experience.

References

Closes #12246
Closes #12168
Closes #12162

Reviewer guidance

General

  • Can create a quiz
  • Can replace questions in the quiz sections (seems When editing a quiz I cannot replace a questions #12246 is fixed here)
  • Can take the quiz and navigate through appropriately
  • Upon submitting the quiz, you see the report as usual
  • Coach can view the learner's quiz report

Mobile-specific

These should apply to both taking the quiz and viewing the reports

  • On small screens, the side-bar accordion of questions should now be hidden and the user has a dropdown list of questions.
  • When there are more than one section, there is a dropdown list for sections too
  • When selecting a section by the dropdown, the first question of that section becomes the "selected question"

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: large labels May 17, 2024
@nucleogenesis nucleogenesis self-assigned this May 21, 2024
@nucleogenesis nucleogenesis force-pushed the enhancement--quiz-section-question-list branch from 1183b13 to d65d6b8 Compare May 28, 2024 19:50
@nucleogenesis nucleogenesis requested a review from rtibbles May 28, 2024 22:06
@nucleogenesis nucleogenesis force-pushed the enhancement--quiz-section-question-list branch 2 times, most recently from a046f94 to 946b42f Compare May 31, 2024 22:31
@nucleogenesis nucleogenesis marked this pull request as ready for review June 4, 2024 05:31
@nucleogenesis nucleogenesis requested a review from pcenov June 4, 2024 05:31
@pcenov
Copy link
Member

pcenov commented Jun 4, 2024

Hi @nucleogenesis seems that there are conflicts that need to be resolved before I can test your latest changes here - the build artefacts above are from 4 days ago?
Nevertheless I also had a look with that asset and here are some minor considerations:

  1. If I input the max number of characters for the Section title I am seeing issues in the editor. Otherwise from a learners point of view it looks ok:
Long.texts.everywhere.mp4
  1. It's possible to save a section without any questions - not sure what is the expected behavior here, but it might look like a glitch from a learner's point of view:
no.questions.mp4

@rtibbles
Copy link
Member

rtibbles commented Jun 4, 2024

@pcenov the empty sections were a deliberate choice, we have a follow up issue to get rid of these at publish time to avoid this showing to learners: #12217

We wanted to leave the empty sections so that someone editing a quiz can leave something as 'work in progress'.

@nucleogenesis nucleogenesis added the TAG: user strings Application text that gets translated label Jun 4, 2024
@nucleogenesis nucleogenesis force-pushed the enhancement--quiz-section-question-list branch from ab359ce to f8b27af Compare June 5, 2024 23:04
@radinamatic
Copy link
Member

Replacing questions so far seems OK, still need to do more poking around, but we're not catching properly when the coach is trying to use the quiz title that already exists:

unique-title

@radinamatic
Copy link
Member

I did not realize this before, and I'm not saying it needs fixing now, but was the decision to contain the scope of the pool of available resources within each section (and not the whole quiz) intentional? In a sense that when a coach wants to replace a question in section 2, they only have available the resources they specifically selected for that section to select from, and if they wanted to use one of the questions from resources selected for section 1, they need to update the resources in section 2 first, and re-select those included in section 1, to be able to pick some questions from it to select for inserting in the section 2. Tad repetitive, imo... 🤔
cc @tomiwaoLE

@radinamatic
Copy link
Member

radinamatic commented Jun 6, 2024

Proposal to simplify the UX writing in this message:

More Fewer
select-more select-fewer

Saying Select N **more** questions to continue (when none are selected) or Select N **fewer** questions to continue sounds convoluted and unnecessarily complicated. Did we consider keeping the same straightforward message Select N questions to continue all the time (with the Replace button inactive) UNTIL the user selects exactly N questions?

Maybe add a ❗ icon at the beginning of that message too, to point the user's attention or similar... cc @tomiwaoLE

@radinamatic
Copy link
Member

And after poking around the replacing questions long enough, I got this message that was blocking me from further editing (logs attached at the bottom):

TypeError qArray is undefined

db-logs.zip

@nucleogenesis
Copy link
Member Author

but was the decision to contain the scope of the pool of available resources within each section (and not the whole quiz) intentional?

I recall mentioning this early on in the design discussion / review process when I was initially structuring things. This was the decided path forward, although I must say I agree with you that this makes for a slightly convoluted workflow.

As it stands now, I think we could do better with how we inform the user about how the resource selection stuff works. Perhaps messages explaining that you're selecting resources for this section if we keep things how they are.

(cc @tomiwaoLE @radinamatic @rtibbles )

@@ -2,7 +2,7 @@

<div class="wrapper">
<h1 class="section-header" :style="{ color: `${$themeTokens.annotation}` }">
{{ activeSection.section_title }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the line of code where Radina's replacements sidepanel was failing... not sure if my changes would help avoid that or not though.

Comment on lines +230 to +223
const activeSectionIndex = allSections.value.findIndex(section =>
isEqual(JSON.stringify(section), JSON.stringify(activeSection.value))
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There have been a couple places like this where I have to work out where in the list of sections the "current" section is and it makes me miss having a unique ID on them.

However, since sections cannot have duplicate questions between them AND a user cannot replace questions until they've added questions... this ought to do just fine unless JSON.stringify isn't as deterministic as I think.

@nucleogenesis nucleogenesis force-pushed the enhancement--quiz-section-question-list branch from c5ae963 to 599460b Compare June 6, 2024 21:56
@@ -17,6 +17,7 @@
:questions="questions"
:isSurvey="isSurvey"
:isQuiz="!isSurvey"
:sections="exam.question_sources"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will never be defined - can be cleaned up once there's a default on the sections prop.

@pcenov
Copy link
Member

pcenov commented Jun 7, 2024

Hi @nucleogenesis,
Here's a list with the issues I was able to find:

  1. Since we are using the default title of 'Section 1' when I open the section settings I am expecting to see that title by default. Currently it's missing and when I attempt to manually enter it I see the 'Section titles must be unique within this quiz' message:
section.title.mp4

The same happens if I just enter any name, click the Apply settings button and then attempt to make and additional changes:

section.mp4
  1. If I leave an interval after the title it can be saved looking as a duplicate. The same is valid if I enter the title with caps lock on:
duplicate.mp4
  1. When deleting a section with the default title, then that title is also missing in the confirmation message:
delete.mp4
  1. I can't select and start a practice quiz:
can.t.select.a.practize.quiz.mp4

@nucleogenesis
Copy link
Member Author

nucleogenesis commented Jun 10, 2024

Thank you @pcenov -- I will make a follow-up issue for cleaning up the over-eager yet-also-failing validations. The practice quiz issue is being addressed in a separate PR by @rtibbles

With that, I think that we'll just need reviewer approval and can get this merged. I think it'd be helpful for #9759 to rebase on my work to see if the latest reported issues there are fixed here

Edit: Follow-up issue

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things that I think could be resolved quickly.

I guess we decided not to allow selection of folders at all? Have we updated that in the selection behaviour as well?

@@ -140,6 +140,7 @@
@submit="handleOpenQuiz(activeQuiz.id)"
>
<p>{{ openQuizModalDetail$() }}</p>
<p>{{ canNoLongerEditQuizNotice$() }}</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be conditionalized on quiz.draft being true - otherwise we will show this warning for quizzes that couldn't be edited anyway.

@@ -522,6 +522,11 @@ const coachStrings = createTranslator('CommonCoachStrings', {
context:
"Text shown on a modal pop-up window when the user clicks the 'Start Quiz' button. This explains what will happen when the user confirms the action of starting the quiz.",
},
canNoLongerEditQuizNotice: {
message: 'You will no longer be able to edit the quiz.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inaccurate - it should be something like "You will no longer be able to edit the questions and sections in this quiz" - as they will still be able to edit the title and assignments.

@@ -73,7 +73,7 @@ export const enhancedQuizManagementStrings = createTranslator('EnhancedQuizManag
message: 'Add questions',
},
selectFoldersOrExercises: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth changing the message id if we're not only allowing selection of resources?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 good thought richard

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers from my perspective on the code review side (although I didn't do any manual QA). Jacob, if you're able to get in the small pieces of feedback from Richard right away, then let's go ahead and merge and iterate from there so we can also get Richard's PR in and give him time to sort through merge conflicts

@@ -73,7 +73,7 @@ export const enhancedQuizManagementStrings = createTranslator('EnhancedQuizManag
message: 'Add questions',
},
selectFoldersOrExercises: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 good thought richard

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick smoke test and workflow walk through seems good, so let's merge and carry on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: large SIZE: very large TAG: user strings Application text that gets translated
Projects
None yet
5 participants