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

Coach Quiz Exercise Resources #11682

Merged

Conversation

nucleogenesis
Copy link
Member

Summary

  • Introduces new useExerciseResources module
  • Implements basic interface for fetching topics & exercises, annotated with their respective number of assessment descendants
  • Hooks that up into previous work done by @AllanOXDi on the Select Resources page so that the user can navigate the tree

Outstanding issues

Some or all of these may not be entirely necessary to implement in this PR, but I've been trying to work out an answer to some of these issues with varying degrees of success:

The router situation is a bit screwy

As it is now the component in which we're intending to render and update the list of content cards is being reloaded/remounted every time the route changes.

The intention for this is that whenever the route changes (specifically, the :topic_id param) then we'll be able to push that onto the state and the browser back/forward navigation functions will allow the user to walk back and forth through the tree as one would expect.

I've played with and tried a few solutions that have tried to sort of tweak what is already here but none of them have been effective.

I've also:

  • Ensured there is no :key on any router-view which would force re-render
  • Tried using named router-views w/ the nested routes (although I feel that maybe I missed something in this implementation w/ the structure of the routes)
  • Tried using <keep-alive> on the router-view in CoachImmersivePage (and the others I tried to use as well)

The result here is that the useExerciseResources module is re-initialized every time the route changes which is Extremely Not Good ™️ for the user or for the developer using the module and relying on it to just do the magic it ought to, update it's refs and allow the components to derive their UI from that state.

ContentNodeResource.fetchTree and the figuring out of checkboxes

The designs expect that we allow checkboxes on topics and I think that this is the ideal solution, with some exceptions.

When I spoke w/ @rtibbles about this we discussed that basically we should just not allow the checking of a topic folder unless we can get all of the possible assessments from within the currently-fetched tree (which is limited to two generations). However, the child content nodes themselves are not annotated with num_assessments which makes figuring out whether or not we can select the topic and all of its children difficult.

I think maybe this would be worth a co-hack at some point and I don't consider solving this to be a blocking issue for this issue as it would be a fine follow-up issue.

Clean-up

  • Remove all channels and bookmarks business from useQuizCreation and anywhere it is used, replace it with those from useExerciseResources (it may not be referenced anywhere else though).
  • Clean-up the coach plan constants' PageNames map, removing the PageNames which we are no longer using

Reviewer Guidance

  • Please look at the useExerciseResources in particular. It only currently handles the basic fetching-and-annotating.
  • Does the overarching structure for the module seem reasonable (see the comments within the module)?

Any and all thoughts & suggestions around the router would be greatly appreciated.

@nucleogenesis nucleogenesis marked this pull request as draft January 1, 2024 06:41
@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: large labels Jan 1, 2024
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 notes on implementation - some cleanup that I think can help consistency of behaviour here.

@nucleogenesis nucleogenesis self-assigned this Jan 8, 2024
nucleogenesis and others added 2 commits January 10, 2024 08:48
Co-authored-by: Allan Otodi Opeto <otodi.allan1@gmail.com>
splits the business of fetching (and fetching more) in the fetchTree
method into its own module

then another new module repurposes code from useExerciseResources and
leverages the new useFetchTree module to fetch the contents and
annotate them as needed for navigation the content tree specifically for
use with quizzes

useFetchTree (at least tries) to straightforwardly provide the intial
fetch of content while keeping itself up-to-date whenever the user
fetches more
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.

Would still like to make this work for a reactive topicId, so that we can reliably use this when the entire component hierarchy is not being torn down on every navigation event.

@nucleogenesis nucleogenesis marked this pull request as ready for review January 13, 2024 00:16
@rtibbles rtibbles self-assigned this Jan 15, 2024
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.

I think all my previous comments have been addressed, mostly questions about the code and next steps, and a little cleanup.

- use kolibri.lib.logging in place of console.*
- remove unneeded console.logs
- add guidance re: private variables
- remove unused search-related business
- fix bookmarks route (no topic_id)
- remove unused pagename constants
- code cleanup thanks to @rtibbles suggestions
- improved comments
- indicated follow-up issues where commented-out TODO work remains
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.

All code comments addressed - manual testing shows no errors apart from 'remains to be implemented'!

@rtibbles
Copy link
Member

Docs build has been fixed independently.

@rtibbles rtibbles merged commit 2e82017 into learningequality:develop Jan 18, 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: very large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants