Initial implementation of the Resource management side panel #12857
Conversation
Build Artifacts
|
There was a problem hiding this comment.
Couple of thoughts from my side. I also observed that when I visit the lessontemps route I have to manually add /select-resources at the end to open side panel. Clicking on manage resources does not seem to be working for me (when I am on lessonstemp route and browsing individual lesson).
| @@ -77,6 +77,7 @@ | |||
| </KPageContainer> | |||
| </KGridItem> | |||
| </KGrid> | |||
| <router-view /> | |||
There was a problem hiding this comment.
Instead of just having a router-view here, we can have the sidePanel too and inside that we can have this router-view. This way we would not need to replicate the SidePanel in other components. Is there a specific reason that you did so or am I missing something here?
There was a problem hiding this comment.
Hi! Yes, there are some motivations for keeping this in this way. The main motivation is to keep these side panels in sync with the same pattern that will be used in quizzes that will be introduced in this PR #12819, and in that PR you can find more motivations for this like:
- Removing unnecessary indirection around when/how/where components define what to do re: back and close behavior
- Providing a better example for handling side panels w/ routing for Lessons
- Much easier to put things into the Side panel title section
At the end its a tradeof of how much do we want to abstract and reuse, and how much flexibility do we need to avoid making the pattern more complex to follow.
There was a problem hiding this comment.
Elaborating on the side panel title thing - basically, if the child-component rendered in the router-view has it's own SidePanelModal then it has the direct ability to set the title prop (or pass to the header slot).
Separately, this way also associates the child-component directly with the route, so the parent component doesn't have to manage a v-if="" for when to show the side panel. The route decides if it's shown automatically without our having to deal adding a conditional in the parent.
Hi @ozer550, for this you can reload the page, and then it will work. This is a weird behaviour of how vue router mounts/unmounts the components on redirect, but reloading the page will work :). |
| @@ -65,6 +65,12 @@ class CoachToolsModule extends KolibriApp { | |||
| PageNames.QUIZ_SECTION_ORDER, | |||
| PageNames.QUIZ_BOOK_MARKED_RESOURCES, | |||
| PageNames.QUIZ_LEARNER_REPORT, | |||
| PageNames.LESSON_SUMMARY, | |||
There was a problem hiding this comment.
We should reconsider this automatic loading dispatch on every redirect @marcellamaki, as this array is becoming each time more longer.
There was a problem hiding this comment.
[not blocking, clarification]: Do you mean that we are continually adding "exceptions" for the skip loading state? And that a different strategy (vs. skipping loading for exceptions) would be better?
There was a problem hiding this comment.
Yes @marcellamaki, we can see how many routes are actually taking advantage of this vs how many are just skipping this. We also need to consider that in some other routes, we have this defaultHandler that the only thing that it does is to dispatch the "notLoading" event. These pages are also basically skipping this loading state.
There was a problem hiding this comment.
yes I definitely agree with that @AlexVelezLl. We have some long standing issues with coach loading state, in terms of our overall strategy, tech debt, and also in-page loading and state management being well organized. You will see some of these in the general maintenance milestone for 0.18. I think it would be productive to revive some of those conversations and make some decisions, and see how we can resolve some of these challenges in the scope of this work where possible (or create a plan for doing it in follow up). thank you for raising this!
| @@ -20,14 +23,18 @@ | |||
| </template> | |||
| <template #belowTitle> | |||
| <div> | |||
| <KTextTruncator | |||
| :text="coachString('numberOfResources', { value: contentNode.num_coach_contents })" | |||
There was a problem hiding this comment.
Here in the current implementation of this AccessibleChannelCard we are using this num_coach_contents to show the number of resources, but Im not that shure if thats completely accurate. I think the number we should be showing here is the total_resource_count of the channel. But havent updated yet just in case the current value has any other motivation @marcellamaki.
There was a problem hiding this comment.
yes, I think it should be total_resource_count @AlexVelezLl
There was a problem hiding this comment.
Just pushed this change!
There was a problem hiding this comment.
thank you @AlexVelezLl! I added a non-blocking question if you can take a look at that, but this is good to go. thank you! If other adjustments come up as necessary when others rebase and build on this, we will just go from there :)
| @@ -65,6 +65,12 @@ class CoachToolsModule extends KolibriApp { | |||
| PageNames.QUIZ_SECTION_ORDER, | |||
| PageNames.QUIZ_BOOK_MARKED_RESOURCES, | |||
| PageNames.QUIZ_LEARNER_REPORT, | |||
| PageNames.LESSON_SUMMARY, | |||
There was a problem hiding this comment.
[not blocking, clarification]: Do you mean that we are continually adding "exceptions" for the skip loading state? And that a different strategy (vs. skipping loading for exceptions) would be better?
Summary
/coach/#/:classId/lessonstemp/:lessonId/select-resources.Compartir.pantalla.-.2024-11-19.12_28_58.mp4
References
Closes #12786.
Reviewer guidance
/coach/#/:classId/lessonstemp/:lessonId/select-resources, and check that the side panel view matches the figma specs.Follow ups