-
Notifications
You must be signed in to change notification settings - Fork 726
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
Add quiz recipients selector as Side Panel #12952
base: develop
Are you sure you want to change the base?
Add quiz recipients selector as Side Panel #12952
Conversation
Build Artifacts
|
@@ -0,0 +1,130 @@ | |||
<template> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a refactor to break down the current IndividualLearnerSelector
component to have an IndividualLearnerSelectorTable
that we can reuse in the LearnersSelectorSidePanel
. I havent edited the logic of this component, just extracted the table.
{{ $tr('selectGroupsAndIndividualLearnersTitle') }} | ||
</h1> | ||
</template> | ||
<div style="padding-top: 30px"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display: flex; | ||
justify-content: flex-end; | ||
width: 100%; | ||
margin-bottom: 16px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks really good @AlexVelezLl - I don't think I'll have time to do a second pass today (so it will be merged after the new year) but on initial read through overall the code changes make sense to me. I'd love for @pcenov to do some more robust QA especially since he is so good at finding those edge cases. If you can get to it tomorrow (Friday 20th) Peter, great, but no problem if it's not til after the holidays as well.
One small UI note - I'm not sure if this may be resolved after we merge your resource selection and sidepanel refactors PR, but the page content for a long learner list is cut off. All of the learners are visible, but because the pagination is covered by the bottom bar, it looks as if something might be "missing" (and presumably, if there were enough learners to paginate, this would prevent the user from being able to). If this will be fixed after rebasing that other PR, please disregard!
after deleting the bottom bar element in the browser:@@ -11,6 +11,10 @@ | |||
{{ submitErrorMessage }} | |||
</UiAlert> | |||
|
|||
<!-- | |||
TODO: Refactor or rename this component, setting a title or a description | |||
is not part of an "assignment" process (?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think calling it something more like "QuizTitleAndRecipientsHeader" would make more sense even if it's longer
@pageChanged="currentPageLearners = $event.items" | ||
> | ||
<template #default="{ items }"> | ||
<CoreTable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is absolutely not required and not blocking, but just a thought, @AlexVelezLl - do you think that using the new KTable
here would be difficult to refactor in? If it's going to add lots of work, we don't have to. I just wonder if there might be any a11y benefits and this is a moment when we could refactor it in "in the course of" rather than in separate work as tech debt tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could spend 30-60 minutes and see how it goes? If it seems like it would take a lot longer and introduce complexities, we don't have to do it now. If it seems like something you could do in a few hours, perhaps worth it (especially as the next couple of weeks will be quite)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure! I can give it a try and see how it goes :).
{ | ||
label: this.$tr('copyQuizAction'), | ||
value: 'COPY', | ||
}, | ||
{ label: this.coreString('deleteAction'), value: 'DELETE' }, | ||
]; | ||
if (!this.exam?.archive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just because the archive
key would exist so we can use that instead of the draft
prop? If so, how is that distinct from this.exam?.draft
that we are using below for the label condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, there are three different "states". exam in draft, exam "active"(once the exam is published) and exam closed. The this.exam?.archive
represents that the exam was closed. So when the exam is "not archived", it can be either in draft or not.
Thanks @marcellamaki. Yes! The issue with the hidden pagination buttons will be fixed once #12895 is merged too :). |
bb801dd
to
ef6d377
Compare
Hi @AlexVelezLl and @marcellamaki, just some comments/points for discussion:
select.all.mp4Also when there aren't any learners yet the checkbox "All ungrouped learners" is selected by default and cannot be deselected:
validation.mp4 |
Also we probably should have the same implementation when copying a quiz, as currently it's not changed there: copy.quiz.mp4 |
Thank you @pcenov - this is all very helpful. The reason for this change was some feedback that we've gotten from the community that @jtamiace created a new design for. I'll chat review your notes with her and make sure that we're addressing the implementation in all of the places that we need, and will also look through the issues/concerns with the validation (and discuss with @AlexVelezLl to figure out the exact criteria we need). Happy new year :) |
Happy new year @marcellamaki! Looking back at my comments I guess the 2024 version of me was just mildly surprised to see the change, but it's a welcome change! :) |
Its not always the same, "All ungrouped learners" refers to all learners that dont belong to any group, so checking and unchecking this will check and uncheck just the learners that dont belong to any group, so:
While "select all on page" will select/deselect all "selectable" learners (bear in mind that learners that has been selected through group selection are disabled and will not be affected by this) whether they are in a group or not, but will just affect the learners that are in the current page.
I havent thought about this 😅, I think we shouldnt even have the radio button if we dont have learners right? @marcellamaki and just always create the quiz in the scope of the whole class. |
Yes, that sounds like a good idea, and it's close to how it currently works in 0.17 (where you can create a quiz assigned to "entire class" if the class has no learners enrolled). |
@pageChanged="currentPageLearners = $event.items" | ||
> | ||
<template #default="{ items }"> | ||
<KTable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AlexVelezLl - thank you for the updates to use KTable
! Overall, I think this is looking good. I never do quite as complete of QA as Peter does, of course, but from my perspective, the code changes look good and I only found one small area of improvement with the manual QA.
I noticed that the validation error banner doesn't dismiss after the error is resolved when testing quiz assignment. i.e.
Screen.Recording.2025-01-09.at.2.28.45.PM.mov
Even though this banner can be manually dismissed, I think having it auto dismiss on resolve would be helpful for the user.
Thanks @marcellamaki! I have pushed the changes revalidating the form on change, and removing the select groups and learners radio button if there are no learners in the class. |
Summary
Add quiz recipients selector as Side Panel.
Compartir.pantalla.-.2024-12-17.10_59_21.mp4
References
Closes #12901.
…
Reviewer guidance