-
Notifications
You must be signed in to change notification settings - Fork 743
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
Integrate shopping cart into lesson resource selection side panel #12996
Integrate shopping cart into lesson resource selection side panel #12996
Conversation
...ns/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/ManageSelectedResources.vue
Show resolved
Hide resolved
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.
A few notes from an initial read through - I'll do a second review tomorrow. I also haven't run this locally yet or tried it out.
@pcenov I'll add you here for manual QA - please let me or Alex know if you have any questions about how to test these changes. Thank you!
...oach/assets/src/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/index.vue
Show resolved
Hide resolved
...oach/assets/src/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/index.vue
Show resolved
Hide resolved
...oach/assets/src/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/index.vue
Outdated
Show resolved
Hide resolved
...oach/assets/src/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/index.vue
Outdated
Show resolved
Hide resolved
Hi @AlexVelezLl and @marcellamaki, I confirm that this is implemented as specified above. Noticed the following potential issues:
Current implementation: selection.mp4Previous: previous.mp4
cannot.preview.a.resource.mp4
the.back.arrow.mp4 |
68422bf
to
58f9ff6
Compare
I think this is desirable - with one change from the previous behaviour - the checkboxes should be marked as selected, but disabled. This is an intentional change we introduced in the quiz workflow, so that resource selection was a 'shopping cart' experience - going to find new resources to include - not a selection and deselection experience. Removing resources is done from the lesson itself, not the resource selection flow. |
For 2 - we have two follow up issues for the preview behaviours, so yes, I think out of scope for this PR! #12988 and #12989 For 3 - my understanding is that this is currently intended behaviour. I think we shouldn't block the PR for this, and I know there are other back arrow confusions in resource selection in quiz too, so we should keep a close eye on people's reactions to this as we do broader internal testing. |
Thank @rtibbles! I have pushed the changes to have these checkboxes selected but disabled :) Compartir.pantalla.-.2025-01-24.09_04_54.mp4 |
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.
Thanks @AlexVelezLl - I confirm that the checkboxes of the already added resources are disabled now.
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 think we could do with a better name for noSelectableResourcesIds
see my comment on one instance.
I have a broader question about state management here, which maybe is better deferred from this PR and addressed to @marcellamaki - it seems like we're still using the lessonSummary vuex state, rather than following the pattern we followed in quiz to start from scratch with a composable for editing.
import SelectionIndex from '../views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectionIndex.vue'; | ||
import SelectFromBookmarks from '../views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectFromBookmarks.vue'; | ||
import SelectFromChannels from '../views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectFromChannels.vue'; | ||
import ManageSelectedResources from '../views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/ManageSelectedResources.vue'; |
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.
Does adding the .vue
suffix help with IDE file discovery? Just wondering if we should standardize on one way or the other. Our webpack config handles discovery of vue files without the extension, but maybe IDEs are not handling it as well?
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, its just for VS Code. without it, VS Code expect it to be a js
file, and therefore, it just dont find the file. Adding the .vue
let us navigate to the file just doing cntrl + click and It is much more comfortable. Although I haven't taken the time to investigate if I can configure VS Code for this.
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 may also be influencing why this is not being supported vitejs/vite#178
Vite is also removing support for imports of vue components without the extension.
We are probably going to be better off bending with the wind!
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.
We are probably going to be better off bending with the wind!
I agree!
kolibri/plugins/coach/assets/src/views/lessons/LessonSummaryPage/UpdatedResourceSelection.vue
Outdated
Show resolved
Hide resolved
PageNames, | ||
}; | ||
}, | ||
computed: { | ||
totalSize() { | ||
...mapState('lessonSummary', ['currentLesson', 'workingResources']), |
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, have we not migrated lesson editing over to using composition API for state? Are we updating the lessonSummary vuex whenever we edit things as well? cc @marcellamaki
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.
Are we updating the lessonSummary vuex whenever we edit things as well?
Yes, thats why I had to call this setWorkingResources
call here.
And in the LessonSummaryPage, we also update this workingResources array when we remove a resource from the lesson
kolibri/kolibri/plugins/coach/assets/src/views/lessons/LessonSummaryPage/index.vue
Line 343 in 0b44855
this.setWorkingResources(resources); |
...ns/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/ManageSelectedResources.vue
Show resolved
Hide resolved
82cf6cf
to
12467f4
Compare
4728873
to
85a925d
Compare
Thanks @rtibbles! I have addressed the comments :) |
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.
Approving since @pcenov has given the 👍 with manual QA, and it looks like Richards remaining feedback has all been addressed. Thanks @AlexVelezLl!
Thanks @marcellamaki! |
Summary
Compartir.pantalla.-.2025-01-13.16_12_02.mp4
References
Closes #12990.
Reviewer guidance
Implementation notes
As we shouldnt have repeated resources in a lesson, I have make sure to save unique resoures in the list. This implies that if a user is selecting a resource that is already in the lesson, it will just "ignore" it (because of the unification process). So it wont take it into account to make the save request, neither to show the close confirmation message if no new resources are being added. Does this makes sense?