-
Notifications
You must be signed in to change notification settings - Fork 699
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
Questions randomly selected from resource pool #11823
Questions randomly selected from resource pool #11823
Conversation
Build Artifacts
|
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.
Some comments - again nothing blocking, and some comments are related to previous questions around where the active exercise pool logic lives.
@@ -272,6 +276,16 @@ export default function useQuizCreation(DEBUG = false) { | |||
setActiveSection(newSection.section_id); | |||
} | |||
_fetchChannels(); | |||
|
|||
// Set watcher once we have a section in place | |||
watch(activeResourcePool, (resourcePool, old) => { |
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.
Presumably we initiate this whenever we do, so we might be able to decide more directly when to do this rather than a potentially expensive or unreliable isEqual call?
Not clear that this is actually a problem, so definitely not a blocker.
* available questions */ | ||
const activeExercisePool = computed(() => get(activeResourcePool).filter(isExercise)); | ||
/** @type {ComputedRef<QuizExercise[]>} The active section's `resource_pool` */ | ||
const activeResourceMap = computed(() => |
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.
The alternative here would be to store an activeResourceMap as the basic representation, and have Object.values
for activeExercisePool
if it is ever needed as an array.
Looks like this is never exported, so maybe we should just set it as a map in the first place?
> | ||
CONTENT OF {{ question.title }} | ||
</p> | ||
<ContentRenderer |
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.
Should probably use the :interactive=false
prop here too: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/assets/src/views/ContentRenderer/mixin.js#L135
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.
I reviewed the workingResourcePool part of the PR and it seems like we still need to do some clean up in the SectionSidepanel component after the place where working resource pool lived has changed. During review I also found a slight mistake from my part where I have directly set the value of computed property isSelectAllChecked
in another computed property called toggleTopicInWorkingResources
. I could also do the cleaup in one of my PR's if you say.
One more behaviour that I doubt was the following:
- I selected some of the exercises in the side panel.
- Navigated back to the channels URL through ResourceSelectionBreadcrums. (same thing happening with every other breadcrum link).
- Instead of seeing the channels I saw the selected exercises in workingResourcPool.
was curious if this is desired behaviour?
Thanks for the review @ozer550 The "selectAllIsChecked" error you mentioned is handled in #11825 by @AllanOXDi so no worries there. As for the Channels link -- I noticed that before and have just added it to #11741 so that we can take care of it in a follow-up -- I think that it would probably be a good idea to take care of that as part of follow-up to your bookmarks PR since it adds something to how we handle routing. In fact, there is a chance that your PR resolves the issue as-is but we'll have to test that out to be sure |
Thanks for the clarification @nucleogenesis! LGTM. |
this was added early on due to misunderstanding around how we'd handle resources, keeping activeResourcePool
… on numQuestions change
6b5c8aa
to
29a97ba
Compare
Summary
selectResources.js
into autils
directory, updates references to itactiveResourceMap
computed property which can be used to reference all items in the active section's resource pool bycontent_id
ContentRenderer
to render questionsQuizExercise
to accurately represent the data we're getting from the backendNOTE re: Imminent Perseus updates
Perseus currently relies on a div tag's ID property to find where to mount the React components it uses. This results in funky rendering of the questions such that when we go to render a question, it always mounts to the first perseus renderer instance.
This will be resolved by some updates needed on #9759 - but that may take some time.
References
Closes #11734
Closes #11276
Closes #11549
Reviewer guidance