Fix: Side panel closing confirmation & logic in child component #11862

nucleogenesis · 2024-02-13T00:38:29Z

Summary

The issue here was that the logic for whether or not the panel should be closed or whether or not we need to show a confirmation modal was initially built into the SectionSidePanel component. This component is a wrapper component and now that the workingResourcePool lives solely within the ResourceSelection component, there is no access to it in the parent.

The solution was to pass a prop into each child component which will need to be handled in all of the child components (section edit, question replacement). This prop tells the child that we're trying to close the side panel.

This allows each child component then to define its own logic and messaging around confirming the panels closure with the user. This results in the child component also needing to emit two events which tell the SectionSidePanel if the panel should actually be closed, resulting in the user being returned to the root quiz creation route.

Reviewer guidance

Testing

Proceed to add questions to a section
Change your selection (click one resource's checkbox)
Try to close the side panel by the X or by clicking outside of the side panel
See the modal window pops up
When "Cancel" is clicked, you still see the side panel and if you try to close again, you get the confirmation modal again
When "Continue" is clicked, you see the side panel closed
Try this again on the same section by clicking "Options" -> "Edit section" -> "Select resources..." and then make a change to your selection and the same confirmation modal flow as noted above should be done.
The same workflow should present itself when in the SectionEditor

AllanOXDi · 2024-02-13T10:43:24Z

Hi @nucleogenesis I am wondering if clicking outside the side panel should trigger the model?

AllanOXDi · 2024-02-13T12:34:21Z

I think this can be a separate issue - I have also discovered that if I select a single resource and click save changes it will add a duplicate of that same resource

nucleogenesis · 2024-02-13T20:48:56Z

@AllanOXDi could you give this another look? I've made a change now so that the SectionEditor also has a working confirmation modal when closing after making changes without saving.

github-actions · 2024-02-13T21:17:43Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0a0.dev0_git.622.gd69f0740.pex
Windows Installer (EXE)	kolibri-0.16.0a0.dev0+git.622.gd69f0740-unsigned.exe
Debian Package	kolibri_0.16.0a0.dev0+git.622.gd69f0740-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0a0.dev0+git.622.gd69f0740-0.4.0.dmg
Android Package (APK)	kolibri-0.16.0a0.dev0+git.622.gd69f0740-0.1.0-debug.apk
TAR file	kolibri-0.16.0a0.dev0+git.622.gd69f0740.tar.gz
WHL file	kolibri-0.16.0a0.dev0+git.622.gd69f0740-py2.py3-none-any.whl

AllanOXDi · 2024-02-14T15:41:05Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ConfirmCancellationModal.vue

I took another look, and it appears that clicking the 'X' is functioning as intended. However, I observed that selecting a resource and then navigating back using the back icon, until I exit the side panel, does not trigger the modal. I believe there might be a need to update the logic within this method.
https://github.com/nucleogenesis/kolibri/blob/fix--side-panel-closure/kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ResourceSelection.vue#L411

nucleogenesis · 2024-02-16T00:07:32Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue

+            learners_see_fixed_order: learners_see_fixed_order.value,
+            question_count: question_count.value,
+            description: description.value,
+            section_title: section_title.value,


Renaming these to match the data saved in the quiz data made it easy to compare the values by way of lodash/pick below

Might be good to add this as a code comment? Good to clarify that snake_case is being used rather than camelCase to match API data.

nucleogenesis · 2024-02-16T00:08:19Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ResourceSelection.vue

@@ -146,6 +145,8 @@
      const { updateSection, activeResourcePool, selectAllQuestions } = injectQuizCreation();
      const showConfirmationModal = ref(false);

+      const prevRoute = ref({ name: PageNames.EXAM_CREATION_ROOT });


This can be set to the SectionEditor during beforeRouteEnter below if that's the way we get here so that we can always go back there whenever we got to the resource selection from there

rtibbles · 2024-02-16T17:23:43Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

@@ -509,13 +505,13 @@
    methods: {
      handleReplaceSelection() {
        const section_id = get(this.activeSection).section_id;
-        this.$router.replace({ path: 'new/' + section_id + '/replace-questions' });
+        this.$router.push({ path: 'new/' + section_id + '/replace-questions' });


I hadn't noticed this before - but is there a reason to use a path here and below, rather than name and param? It feels a little less brittle to do the latter, so if we need to tweak URLs for whatever reason, this remains unchanged.

Thanks! I noticed that in passing yesterday while focused on another part of the code and failed to come back to fix it up. Will fix shortly

AllanOXDi

There is something wired happening with this icon and I don't know why https://github.com/nucleogenesis/kolibri/blob/fix--side-panel-closure/kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ResourceSelection.vue#L14

nucleogenesis · 2024-02-26T23:06:34Z

@AllanOXDi the double-button issue will be fixed in a separate PR - I have it noted and will be sure it's resolved

AllanOXDi

LGTM!

AllanOXDi · 2024-02-27T14:10:33Z

kolibri/plugins/coach/assets/src/routes/planExamRoutes.js

+            name: PageNames.QUIZ_SECTION_EDITOR,
+            path: 'edit',
+            component: SectionEditor,
+          },


None blocking comment: curious why we had to re-introduce nested routes when Richard suggested not to use it?

@rtibbles initial note about the nested routes I think was more of a warning as things can get squirrely in the nested routes when not done correctly.

I was motivated to try the nested approach again because of the routing fix that stopped the page reloading on every navigation and because I found it impossible to reliably capture the browser back event before closing the side panel in order to show the confirmation modal before letting the user close the panel... unless each of the pages were their own route because then I could use beforeRoute* guards within the components.

Previously, I couldn't do beforeRouteLeave on the side panel components because each route just rendered the same quiz root component so the only place beforeRoute* stuff worked was on that root component which only would happen when leaving the quiz creation altogether.

With the nested structure, however, I was able to clean up the components' hierarchies in the side panel. The key thing is that because each route is being associated with a specific component, we can block between all of routes by way of the beforeRouteLeave guard and define that directly in each of the SectionEditor, ResourceSelection, & ReplaceQuestions components themselves. This has the added benefit that they can handle the logic of when they show the modal or not themselves rather than trying to pass values up to the base side panel component.

I see. Good job and thanks for explaining!

The issue here was that the logic for whether or not the panel should be closed or whether or not we need to show a confirmation modal was initially built into the SectionSidePanel component. This component is a wrapper component and now that the workingResourcePool lives solely within the ResourceSelection component, there is no access to it in the parent. The solution was to pass a prop into each child component which will need to be handled in all of the child components (section edit, question replacement). This prop tells the child that we're trying to close the side panel. This allows each child component then to define its own logic and messaging around confirming the panels closure with the user. This results in the child component also needing to emit two events which tell the SectionSidePanel if the panel should actuall be closed, resulting in the user being returned to the root quiz creation route.

…t objects

…e when closing

…en needed

The issue here was that the logic for whether or not the panel should be closed or whether or not we need to show a confirmation modal was initially built into the SectionSidePanel component. This component is a wrapper component and now that the workingResourcePool lives solely within the ResourceSelection component, there is no access to it in the parent. The solution was to pass a prop into each child component which will need to be handled in all of the child components (section edit, question replacement). This prop tells the child that we're trying to close the side panel. This allows each child component then to define its own logic and messaging around confirming the panels closure with the user. This results in the child component also needing to emit two events which tell the SectionSidePanel if the panel should actuall be closed, resulting in the user being returned to the root quiz creation route.

…t objects

…e when closing

nucleogenesis added the TODO: needs review Waiting for review label Feb 13, 2024

nucleogenesis added this to the Kolibri 0.17: Enhanced Quiz Management milestone Feb 13, 2024

nucleogenesis requested review from rtibbles and ozer550 February 13, 2024 00:38

github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Feb 13, 2024

nucleogenesis requested a review from AllanOXDi February 13, 2024 00:38

github-actions bot added the SIZE: small label Feb 13, 2024

rtibbles assigned rtibbles and AllanOXDi and unassigned rtibbles Feb 13, 2024

github-actions bot added the SIZE: medium label Feb 13, 2024

AllanOXDi requested changes Feb 14, 2024

View reviewed changes

marcellamaki requested a review from radinamatic February 14, 2024 23:43

nucleogenesis commented Feb 16, 2024

View reviewed changes

nucleogenesis mentioned this pull request Feb 16, 2024

Quiz Creation Select Resources - Keyword search #11887

Merged

9 tasks

rtibbles reviewed Feb 16, 2024

View reviewed changes

rtibbles mentioned this pull request Feb 18, 2024

Unable to Close Panel After Adding Question with Empty Content in Channel #11901

Open

AllanOXDi requested changes Feb 23, 2024

View reviewed changes

nucleogenesis force-pushed the fix--side-panel-closure branch from 0330db9 to abb3db3 Compare February 26, 2024 22:24

nucleogenesis mentioned this pull request Feb 26, 2024

Quiz creation resource selection: Topic selection & "Select all" #11864

Merged

nucleogenesis requested a review from AllanOXDi February 26, 2024 23:25

AllanOXDi approved these changes Feb 27, 2024

View reviewed changes

AllanOXDi reviewed Feb 27, 2024

View reviewed changes

nucleogenesis requested a review from rtibbles February 27, 2024 16:23

nucleogenesis added 17 commits February 27, 2024 09:41

simplify comparison; recalling that the pools are now lists of IDs no…

ac67c8d

…t objects

remove unused property

e542bce

flip the boolean checking if resources have changed

5271380

add confirmation modal to sectioneditor, always set panelClosing=fals…

edb1257

…e when closing

save channels when they're loaded, then set the content list to it wh…

468cb26

…en needed

$route watcher params better match router terminology

86ada9c

leverage vue-router nested routes for simplified side panel routing

6216dc5

use route names for less brittle routing

87761d3

simplify comparison; recalling that the pools are now lists of IDs no…

60a3f69

…t objects

flip the boolean checking if resources have changed

99e045c

add confirmation modal to sectioneditor, always set panelClosing=fals…

6e0ae16

…e when closing

leverage vue-router nested routes for simplified side panel routing

124a670

fix post-rebase

4b3b75e

add comment explaining use of snake_case in js context

a5eba12

lint

dae568b

nucleogenesis force-pushed the fix--side-panel-closure branch from 1d2e1af to dae568b Compare February 27, 2024 17:44

nucleogenesis merged commit d82f478 into learningequality:develop Feb 27, 2024
31 checks passed

nucleogenesis deleted the fix--side-panel-closure branch February 27, 2024 18:08

nucleogenesis restored the fix--side-panel-closure branch March 5, 2024 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Side panel closing confirmation & logic in child component #11862

Fix: Side panel closing confirmation & logic in child component #11862

nucleogenesis commented Feb 13, 2024 •

edited

Loading

AllanOXDi commented Feb 13, 2024

AllanOXDi commented Feb 13, 2024

nucleogenesis commented Feb 13, 2024

github-actions bot commented Feb 13, 2024 •

edited

Loading

AllanOXDi Feb 14, 2024

nucleogenesis Feb 16, 2024

rtibbles Feb 16, 2024

nucleogenesis Feb 16, 2024 •

edited

Loading

rtibbles Feb 16, 2024

nucleogenesis Feb 16, 2024

AllanOXDi left a comment •

edited

Loading

nucleogenesis commented Feb 26, 2024

AllanOXDi left a comment

AllanOXDi Feb 27, 2024

nucleogenesis Feb 27, 2024

AllanOXDi Feb 27, 2024

Fix: Side panel closing confirmation & logic in child component #11862

Fix: Side panel closing confirmation & logic in child component #11862

Conversation

nucleogenesis commented Feb 13, 2024 • edited Loading

Summary

Reviewer guidance

Testing

AllanOXDi commented Feb 13, 2024

AllanOXDi commented Feb 13, 2024

nucleogenesis commented Feb 13, 2024

github-actions bot commented Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nucleogenesis Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanOXDi left a comment • edited Loading

Choose a reason for hiding this comment

nucleogenesis commented Feb 26, 2024

AllanOXDi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nucleogenesis commented Feb 13, 2024 •

edited

Loading

github-actions bot commented Feb 13, 2024 •

edited

Loading

nucleogenesis Feb 16, 2024 •

edited

Loading

AllanOXDi left a comment •

edited

Loading