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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
c27bd8f
fix: side panel closing confirmation & logic in child component
nucleogenesis Feb 13, 2024
ac67c8d
simplify comparison; recalling that the pools are now lists of IDs no…
nucleogenesis Feb 13, 2024
e542bce
remove unused property
nucleogenesis Feb 13, 2024
5271380
flip the boolean checking if resources have changed
nucleogenesis Feb 13, 2024
edb1257
add confirmation modal to sectioneditor, always set panelClosing=fals…
nucleogenesis Feb 13, 2024
468cb26
save channels when they're loaded, then set the content list to it wh…
nucleogenesis Feb 15, 2024
86ada9c
$route watcher params better match router terminology
nucleogenesis Feb 15, 2024
6216dc5
leverage vue-router nested routes for simplified side panel routing
nucleogenesis Feb 15, 2024
87761d3
use route names for less brittle routing
nucleogenesis Feb 16, 2024
24ad9bb
fix: side panel closing confirmation & logic in child component
nucleogenesis Feb 13, 2024
60a3f69
simplify comparison; recalling that the pools are now lists of IDs no…
nucleogenesis Feb 13, 2024
99e045c
flip the boolean checking if resources have changed
nucleogenesis Feb 13, 2024
6e0ae16
add confirmation modal to sectioneditor, always set panelClosing=fals…
nucleogenesis Feb 13, 2024
124a670
leverage vue-router nested routes for simplified side panel routing
nucleogenesis Feb 15, 2024
4b3b75e
fix post-rebase
nucleogenesis Feb 26, 2024
a5eba12
add comment explaining use of snake_case in js context
nucleogenesis Feb 26, 2024
dae568b
lint
nucleogenesis Feb 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions kolibri/plugins/coach/assets/src/constants/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export const PageNames = {
EXAM_CREATION_PREVIEW: 'EXAM_CREATION_PREVIEW',
EXAM_CREATION_SEARCH: 'EXAM_CREATION_SEARCH',
EXAM_CREATION_QUESTION_SELECTION: 'EXAM_CREATION_QUESTION_SELECTION',
EXAM_SIDE_PANEL: 'EXAM_SIDE_PANEL',
EXAM_PREVIEW: 'EXAM_PREVIEW',
EXAM_REPORT: 'EXAM_REPORT',
EXAM_REPORT_DETAIL: 'EXAM_REPORT_DETAIL',
Expand Down
48 changes: 28 additions & 20 deletions kolibri/plugins/coach/assets/src/routes/planExamRoutes.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ import {
import CreatePracticeQuizPage from '../views/plan/CreateExamPage/CreatePracticeQuizPage.vue';
import CreateExamPage from '../views/plan/CreateExamPage';
import CreateExamPreview from '../views/plan/CreateExamPage/CreateExamPreview.vue';
import SectionSidePanel from '../views/plan/CreateExamPage/SectionSidePanel.vue';
import SectionEditor from '../views/plan/CreateExamPage/SectionEditor.vue';
import ResourceSelection from '../views/plan/CreateExamPage/ResourceSelection.vue';
import ReplaceQuestions from '../views/plan/CreateExamPage/ReplaceQuestions.vue';
import PlanQuizPreviewPage from '../views/plan/PlanQuizPreviewPage';
import CoachExamsPage from '../views/plan/CoachExamsPage';
import { showExamsPage } from '../modules/examsRoot/handlers';
Expand All @@ -37,26 +41,30 @@ export default [
name: PageNames.EXAM_CREATION_ROOT,
path: '/:classId/plan/quizzes/new',
component: CreateExamPage,
},
{
name: PageNames.QUIZ_SECTION_EDITOR,
path: '/:classId/plan/quizzes/new/:section_id/edit',
component: CreateExamPage,
},
{
name: PageNames.QUIZ_REPLACE_QUESTIONS,
path: '/:classId/plan/quizzes/new/:section_id/replace-questions',
component: CreateExamPage,
},
{
name: PageNames.QUIZ_SELECT_RESOURCES,
path: '/:classId/plan/quizzes/new/:section_id/select-resources/:topic_id?',
component: CreateExamPage,
},
{
name: PageNames.BOOK_MARKED_RESOURCES,
path: '/:classId/plan/quizzes/new/:section_id/select-resources/bookmarks',
component: CreateExamPage,
children: [
{
name: PageNames.EXAM_SIDE_PANEL,
path: ':section_id',
component: SectionSidePanel,
children: [
{
name: PageNames.QUIZ_SECTION_EDITOR,
path: 'edit',
component: SectionEditor,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Good job and thanks for explaining!

{
name: PageNames.QUIZ_REPLACE_QUESTIONS,
path: 'replace-questions',
component: ReplaceQuestions,
},
{
name: PageNames.QUIZ_SELECT_RESOURCES,
path: 'select-resources/:topic_id?',
component: ResourceSelection,
},
],
},
],
},
{
name: PageNames.EXAM_CREATION_PRACTICE_QUIZ,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,9 @@
export default {
name: 'ConfirmCancellationModal',
mixins: [commonCoreStrings],
props: {
closePanelRoute: {
type: Object,
required: true,
},
},
methods: {
handleContinueAction() {
this.$emit('continue');
this.$router.replace(this.closePanelRoute);
},
closeModal() {
this.$emit('cancel');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,8 +313,6 @@

</KTabsPanel>

<SectionSidePanel @closePanel="focusActiveSectionTab()" />


</div>

Expand All @@ -334,7 +332,7 @@
import Draggable from 'kolibri.coreVue.components.Draggable';
import { injectQuizCreation } from '../../../composables/useQuizCreation';
import commonCoach from '../../common';
import SectionSidePanel from './SectionSidePanel';
import { PageNames } from '../../../constants';
import TabsWithOverflow from './TabsWithOverflow';
import AccordionContainer from './AccordionContainer';
import AccordionItem from './AccordionItem';
Expand All @@ -351,7 +349,6 @@
DragSortWidget,
DragHandle,
TabsWithOverflow,
SectionSidePanel,
},
mixins: [commonCoreStrings, commonCoach],
setup() {
Expand Down Expand Up @@ -509,13 +506,15 @@
methods: {
handleReplaceSelection() {
const section_id = get(this.activeSection).section_id;
this.$router.replace({ path: 'new/' + section_id + '/replace-questions' });
const route = this.$router.getRoute(PageNames.QUIZ_REPLACE_QUESTIONS, { section_id });
this.$router.push(route);
},
handleActiveSectionAction(opt) {
const section_id = this.activeSection.section_id;
const editRoute = this.$router.getRoute(PageNames.QUIZ_SECTION_EDITOR, { section_id });
switch (opt.label) {
case this.editSectionLabel$():
this.$router.replace({ path: 'new/' + section_id + '/edit' });
this.$router.push(editRoute);
break;
case this.deleteSectionLabel$():
this.removeSection(this.activeSection.section_id);
Expand Down Expand Up @@ -573,7 +572,8 @@
set(this.dragActive, true);
},
openSelectResources(section_id) {
this.$router.replace({ path: 'new/' + section_id + '/select-resources' });
const route = this.$router.getRoute(PageNames.QUIZ_SELECT_RESOURCES, { section_id });
this.$router.push(route);
},
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,18 @@
style="float: right;"
:text="coreString('saveChangesAction')"
:primary="true"
:disabled="!hasTopicId() && !showBookmarks"
:disabled="!workingPoolHasChanged"
@click="saveSelectedResource"
/>
</KGridItem>
</KGrid>
</div>
</div>
<ConfirmCancellationModal
v-if="showConfirmationModal"
@cancel="handleCancelClose"
@continue="handleConfirmClose"
/>
</div>

</template>
Expand All @@ -133,6 +138,7 @@
import { injectQuizCreation } from '../../../composables/useQuizCreation';
import LessonsSearchBox from '../LessonResourceSelectionPage/SearchTools/LessonsSearchBox.vue';
import ContentCardList from './../LessonResourceSelectionPage/ContentCardList.vue';
import ConfirmCancellationModal from './ConfirmCancellationModal.vue';
import ResourceSelectionBreadcrumbs from './../LessonResourceSelectionPage/SearchTools/ResourceSelectionBreadcrumbs.vue';

export default {
Expand All @@ -142,9 +148,10 @@
BookmarkIcon,
LessonsSearchBox,
ResourceSelectionBreadcrumbs,
ConfirmCancellationModal,
},
mixins: [commonCoreStrings],
setup() {
setup(_, context) {
const store = getCurrentInstance().proxy.$store;
const route = computed(() => store.state.route);
const topicId = computed(() => route.value.params.topic_id);
Expand All @@ -154,6 +161,9 @@
const showBookmarks = computed(() => route.value.query.showBookmarks);
const searchQuery = computed(() => route.value.query.search);
const { updateSection, activeResourcePool, selectAllQuestions } = injectQuizCreation();
const showConfirmationModal = ref(false);

const prevRoute = ref({ name: PageNames.EXAM_CREATION_ROOT });
Copy link
Member Author

@nucleogenesis nucleogenesis Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const {
sectionSettings$,
Expand Down Expand Up @@ -377,6 +387,7 @@
// call this annotateTopicsWithDescendantCounts method to ensure that the channels are
// annotated with their num_assessments and those without assessments are filtered out
annotateTopicsWithDescendantCounts(resources.value.map(c => c.id)).then(() => {
channels.value = resources.value;
_loading.value = false;
});
});
Expand Down Expand Up @@ -408,6 +419,10 @@
return searchResults.value;
}

if (!topicId.value) {
return channels.value;
}

return resources.value;
});

Expand All @@ -432,16 +447,36 @@
return fetchMoreQuizResources();
}

function handleCancelClose() {
showConfirmationModal.value = false;
}

function handleConfirmClose() {
context.emit('closePanel');
}

const workingPoolHasChanged = computed(() => {
return (
workingResourcePool.value.length != activeResourcePool.value.length ||
!isEqual(workingResourcePool.value.sort(), activeResourcePool.value.sort())
);
});

return {
selectAllChecked,
selectAllIndeterminate,
showSelectAll,
handleSelectAll,
toggleSelected,
prevRoute,
workingPoolHasChanged,
handleConfirmClose,
handleCancelClose,
topic,
topicId,
contentList,
resources,
showConfirmationModal,
hasCheckbox,
loading,
hasMore,
Expand All @@ -464,18 +499,13 @@
updateSection,
selectAllQuestions,
workingResourcePool,
activeResourcePool,
addToWorkingResourcePool,
removeFromWorkingResourcePool,
showBookmarks,
selectedResourcesInformation$,
};
},
props: {
closePanelRoute: {
type: Object,
required: true,
},
},
computed: {
isTopicIdSet() {
return this.$route.params.topic_id;
Expand Down Expand Up @@ -506,6 +536,19 @@
this.bookmarksCount = newVal.length;
},
},
beforeRouteEnter(_, from, next) {
next(vm => {
vm.prevRoute = from;
});
},
beforeRouteLeave(_, __, next) {
if (!this.showConfirmationModal && this.workingPoolHasChanged) {
this.showConfirmationModal = true;
next(false);
} else {
next();
}
},
methods: {
showTopicSizeWarningCard(content) {
return !this.hasCheckbox(content) && content.kind === ContentNodeKinds.TOPIC;
Expand Down Expand Up @@ -547,9 +590,6 @@
topicsLink(topicId) {
return this.topicListingLink({ ...this.$route.params, topicId });
},
hasTopicId() {
return Boolean(this.$route.params.topic_id);
},
saveSelectedResource() {
this.updateSection({
section_id: this.$route.params.section_id,
Expand All @@ -559,7 +599,7 @@
//Also reset workingResourcePool
this.resetWorkingResourcePool();

this.$router.replace(this.closePanelRoute);
this.$router.replace(this.prevRoute);
},
selectionMetadata(content) {
if (content.kind === ContentNodeKinds.TOPIC) {
Expand Down
Loading