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

First pass string updates following UX writing and ditto review #12312

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Jun 18, 2024

Summary

References

General updates to the add questions UI with alternative validation
Screenshot 2024-06-19 at 7 43 27 PM

When you have selected > 500 questions total
Screenshot 2024-06-19 at 7 43 48 PM

When you have a reasonable number of questions
Screenshot 2024-06-19 at 7 44 07 PM

Showing the number of replacements available
Screenshot 2024-06-19 at 7 44 18 PM

Selecting more questions that replacements available disables replacements
Screenshot 2024-06-19 at 7 44 26 PM

<= number of replacements means replacement button is enabled
Screenshot 2024-06-19 at 7 44 35 PM


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

// message:
// "You've selected { selected, number } { selected, plural, one { question } other { questions } } to replace, but {available, plural, =0 { don't have questions } one { only have 1 question } other { only have { available } questions } } available to replace them with.",
// context:
// 'Message of modal when a user tries to replace more questions than are available in the pool',
Copy link
Member Author

Choose a reason for hiding this comment

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

after doing manual QA will come back to this and make sure all scenarios are still covered

@@ -216,14 +209,14 @@ export const enhancedQuizManagementStrings = createTranslator('EnhancedQuizManag
},
cannotSelectSomeTopicWarning: {
message:
'You can only select folders with { count, number } or less exercises and no subfolders to avoid oversized quizzes.',
'You can only select folders with 12 or fewer exercises.',
Copy link
Member Author

Choose a reason for hiding this comment

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

@rtibbles @nucleogenesis - do we want to keep this (and the "max number of questions 50" string) with this flexible syntax? maybe it's worth it it just seemed like we are not actually planning to add dynamic counts in either place

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I deliberately updated it from the static string that was here previously to give us the flexibility to change this to whatever made the most sense.

Copy link
Member

Choose a reason for hiding this comment

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

The other thing here is whether this criterion is even the correct one - we could change it to just do it based on the number of questions available in the folder.

@@ -85,21 +85,18 @@ export const enhancedQuizManagementStrings = createTranslator('EnhancedQuizManag
message: '{count, number} {count, plural, one {question selected} other {questions selected}}',
},
maxNumberOfQuestions: {
message: 'Max { count, number } { count, plural, one { question } other { questions }}',
message: 'Maximum number of questions is 50',
Copy link
Member Author

Choose a reason for hiding this comment

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

@rtibbles would we prefer to keep this as a dynamic option, rather than changing?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, because I still think we should revisit whether 50 is the correct maximum - we've just assumed it, but I also think the user experience of a 50 max is terrible.

Copy link
Member

Choose a reason for hiding this comment

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

This parameterization still needs to be reinstated.

@@ -18,7 +18,7 @@ export const enhancedQuizManagementStrings = createTranslator('EnhancedQuizManag
message: 'Quiz title',
},
addQuizSections: {
message: 'Add one or more sections to the quiz, according to your needs',
message: 'Add one or more sections, according to your needs',
Copy link
Member

Choose a reason for hiding this comment

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

My general thought about this string, is whether it is needed at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have the same thought

Copy link
Member

Choose a reason for hiding this comment

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

I have no objections to eliminate this string! 💥

// 'Message of modal when a user tries to replace more questions than are available in the pool',
// },
goBackAndUnselect: {
message: 'You can go back and unselect some questions to be able to perform the replacement.',
Copy link
Member

Choose a reason for hiding this comment

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

The only problem here is that at no point now are we telling the user how many replacements there are available, so they don't know how many to select.

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 was a lot of discussion here about whether or not adding the numbers was helpful, or made things more confusing. I don't know if we will reach consensus... My thought is that we pick a direction and either we revisit in 0.18 when some of the workflow changes in general, or try to get some feedback on during training/observations or from our community? (maybe it's a bit granular to expect commentary on)

@marcellamaki marcellamaki force-pushed the post-ditto-review-strings-and-ui-changes branch from 1562488 to 15d16b9 Compare June 19, 2024 23:33
@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium labels Jun 19, 2024
@marcellamaki marcellamaki force-pushed the post-ditto-review-strings-and-ui-changes branch from f2942c9 to 5a50ffe Compare June 19, 2024 23:47
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 few things that are still broken here, and some strings not updated, most importantly.

@@ -198,3 +198,5 @@ export const Presets = Object.freeze({
// This should be kept in sync with the value in
// kolibri/core/exams/constants.py
export const MAX_QUESTIONS_PER_QUIZ_SECTION = 50;
// this is not used for the actual exam model
export const MAX_QUESTION_OPTIONS_PER_QUIZ_SECTION = 500;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should just define this as a multiple of the constant above - either that, or it be dynamic depending on how many questions you are trying to select. But this is not required for string freeze.

/>
</div>
</div>
<h5 class="select-folder-style">
Copy link
Member

Choose a reason for hiding this comment

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

Regardless of whether it's in a KGridItem or not, it still needs to be in a v-else block, as this stuff isn't relevant for the 'Select Quiz' workflow.

@@ -85,21 +85,18 @@ export const enhancedQuizManagementStrings = createTranslator('EnhancedQuizManag
message: '{count, number} {count, plural, one {question selected} other {questions selected}}',
},
maxNumberOfQuestions: {
message: 'Max { count, number } { count, plural, one { question } other { questions }}',
message: 'Maximum number of questions is 50',
Copy link
Member

Choose a reason for hiding this comment

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

This parameterization still needs to be reinstated.

backgroundColor: $themePalette.grey.v_100,
}"
>
{{ cannotSelectSomeTopicWarning$({ count: 500 }) }}
Copy link
Member

Choose a reason for hiding this comment

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

I am not seeing any updates to this string, which presumably will now say you can't select folders with more than 500 exercises instead.

May also be good to update the message id to be more about number of questions?

Copy link
Member Author

Choose a reason for hiding this comment

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

The updated string is this:

'You can only select entire folders when they have { count, number } questions or fewer.',

Is that what you mean?

although this does make me realize I should probably be using the const.... _const_istently! and not just adding 500 in random places

@marcellamaki marcellamaki force-pushed the post-ditto-review-strings-and-ui-changes branch from 1759a06 to fb7b65c Compare June 20, 2024 00:15
@marcellamaki marcellamaki force-pushed the post-ditto-review-strings-and-ui-changes branch from 59e2ac5 to 8966326 Compare June 20, 2024 00:38
@pcenov
Copy link
Member

pcenov commented Jun 20, 2024

Hi @radinamatic,
As discussed here is a screenshot showing the potential confusion caused by the 'selected' word.

confusing number of questions selected vs available

Also as discussed it's currently not possible to replace questions in this build so that's blocking the testing of some scenarios:

Cannot.repleace.a.question.mp4

I'll file the other observed issues separately.

@radinamatic
Copy link
Member

To add to @pcenov's comment, here is another example where the confusion is apparent:

2024-06-20_15-29-59

18 selected questions were actually selected and added in previous sections, but they are also reported as selected in the new, (third) section, which did not actually have any (0 questions selected at the bottom) selected at that point. That is very confusing.

@radinamatic
Copy link
Member

If we could keep the count of questions in the card referring only to questions included in the previous section(s), and the counter at the bottom referring to questions actually selected for the current section, it might alleviate the confusion.

@radinamatic
Copy link
Member

And please, could we revert the default number of questions in the input field to 0? 🙏🏽

@rtibbles
Copy link
Member

And please, could we revert the default number of questions in the input field to 0?

We should not default an input value to an unusable value - then it is not a default. The default of 10 is a sensible value for a reasonable number of questions to add to the section, which can save users from an extra decision if they are in a rush, which we know many of the teachers using our tools are.

@rtibbles
Copy link
Member

If we could keep the count of questions in the card referring only to questions included in the previous section(s), and the counter at the bottom referring to questions actually selected for the current section, it might alleviate the confusion.

Yes, I think this makes sense - I would suggest that we update the wording on the card to X questions in quiz, Y total or something like that?

@radinamatic
Copy link
Member

We should not default an input value to an unusable value - then it is not a default. The default of 10 is a sensible value for a reasonable number of questions to add to the section, which can save users from an extra decision if they are in a rush, which we know many of the teachers using our tools are.

Maybe I'm too nitpicky, but I strongly dislike (and especially in a rush) when the decisions are taken for me (and from me) by an inanimate tool (or its makers). I see the usefulness of the value 0 as the most straightforward (usable) message that the there is still a decision pending on my part on how many questions I need to add, and that I should not proceed until I make that determination. However, I guess we could only exit this impasse with a proper user testing.

@rtibbles
Copy link
Member

However, I guess we could only exit this impasse with a proper user testing.

Yes, and a quick skim of all the feedback we've ever had about quizzes - we've never had a request to remove the existing default of 10 questions for a quiz, which this is a continuation of. If anything, the main requests have been asking for more automation, not less.

@marcellamaki
Copy link
Member Author

Yes, @radinamatic I totally see what you're saying, and I think I may personally lean the same way, but I also think @rtibbles has offered a good reminder that we have gotten feedback around "plz more automatic", and that the current default behavior is 10 questions (even though, I agree that somehow it feels a bit different in this scenario). I think let's stick with it for now and see what feedback we get! We can make adjustments in 0.18, as we'll continue to update these workflows.

@radinamatic
Copy link
Member

I would suggest that we update the wording on the card to X questions in quiz, Y total or something like that?

Would this be good: X questions already used, of Y previously selected? Not too long?

@rtibbles
Copy link
Member

The other thing that this workflow allows, at least, is to more easily and granularly change your mind - by deleting specific questions, or adding more, so hopefully that reduces the frustration for the @marcellamaki and @radinamatic user types out there!

@marcellamaki
Copy link
Member Author

Simplified wording on the replace questions flow:

  • if the the replacements are "equal" - string reflects this and button is in available state
Screenshot 2024-06-20 at 2 00 31 PM
  • if there are an unequal number of replacements selected (more or less) Select X to continue is displayed, and button is in inactive state
Screenshot 2024-06-20 at 2 00 38 PM Screenshot 2024-06-20 at 2 00 23 PM

@radinamatic
Copy link
Member

Yeah, that might be the simplest solution at this point...

@radinamatic
Copy link
Member

It still stutters, when more than required are selected, and does not update the count properly when one by one checkboxes are being de-selected:

count-deselect-flop.mp4

@radinamatic
Copy link
Member

Ok, the above is not string-related, so we should be ready to go! 🎉

},
noResourcesAvailable: {
message:
'There are no resources on your device yet. Ask an administrator to add resources to your device.',
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have these 2 strings already in some common strings file?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

No

Copy link
Member Author

Choose a reason for hiding this comment

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

Similar ones, but neither of these :)

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

Ready for string freeze lift-off! 🚀 ✈️ 🎉 :shipit:

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.

Nothing blocking here, have suggested a couple of tweaks to @marcellamaki directly.

@rtibbles rtibbles merged commit d887f89 into learningequality:develop Jun 20, 2024
30 of 31 checks passed
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.) DEV: frontend SIZE: large SIZE: medium SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants