-
Notifications
You must be signed in to change notification settings - Fork 713
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
Quiz creation resource selection: Topic selection & "Select all" #11864
Merged
nucleogenesis
merged 6 commits into
learningequality:develop
from
nucleogenesis:fix--topics-checkbox-and-select-all
Feb 27, 2024
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
a1ab926
fix: checkbox logic to setup(); topic selection logic implemented
nucleogenesis ecb5920
add messages; slots for showing them w/out issues in other usages of …
nucleogenesis 0d60d8a
remove notification on each card
nucleogenesis 00d40c9
improve spacing of top notice
nucleogenesis 37f2cba
fix select all when topics are selectable
nucleogenesis c6e8450
messaging update & clean-up for topic selection warning
nucleogenesis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,14 @@ | |
</div> | ||
</div> | ||
|
||
<div | ||
v-if="showTopicSizeWarning()" | ||
class="shadow" | ||
:style=" { padding: '1em', marginBottom: '1em', backgroundColor: $themePalette.grey.v_100 }" | ||
> | ||
{{ cannotSelectSomeTopicWarning$() }} | ||
</div> | ||
|
||
<ResourceSelectionBreadcrumbs | ||
v-if="isTopicIdSet" | ||
:ancestors="topic.ancestors" | ||
|
@@ -62,18 +70,19 @@ | |
@clear="clearSearchTerm" | ||
@searchterm="handleSearchTermChange" | ||
/> | ||
|
||
<ContentCardList | ||
:contentList="contentList" | ||
:showSelectAll="true" | ||
:showSelectAll="showSelectAll" | ||
:viewMoreButtonState="viewMoreButtonState" | ||
:selectAllChecked="isSelectAllChecked" | ||
:selectAllChecked="selectAllChecked" | ||
:selectAllIndeterminate="selectAllIndeterminate" | ||
:contentIsChecked="contentPresentInWorkingResourcePool" | ||
:contentHasCheckbox="hasCheckbox" | ||
:contentCardMessage="selectionMetadata" | ||
:contentCardLink="contentLink" | ||
:selectAllIndeterminate="selectAllIndeterminate" | ||
:loadingMoreState="loadingMore" | ||
@changeselectall="toggleTopicInWorkingResources" | ||
@changeselectall="handleSelectAll" | ||
@change_content_card="toggleSelected" | ||
@moreresults="fetchMoreResources" | ||
/> | ||
|
@@ -154,6 +163,7 @@ | |
numberOfSelectedResources$, | ||
numberOfResources$, | ||
selectedResourcesInformation$, | ||
cannotSelectSomeTopicWarning$, | ||
} = enhancedQuizManagementStrings; | ||
|
||
// TODO let's not use text for this | ||
|
@@ -178,7 +188,29 @@ | |
* a list of unique resources to avoid unnecessary duplication | ||
*/ | ||
function addToWorkingResourcePool(resources = []) { | ||
workingResourcePool.value = uniqWith([...workingResourcePool.value, ...resources], isEqual); | ||
workingResourcePool.value = uniqWith( | ||
[ | ||
...workingResourcePool.value, | ||
...resources.filter(r => r.kind === ContentNodeKinds.EXERCISE), | ||
], | ||
isEqual | ||
); | ||
} | ||
|
||
/** | ||
* @description Returns the list of Exercises which can possibly be selected from the current | ||
* contentList taking into consideration the logic for whether a topic can be selected or not. | ||
* @returns {QuizExercise[]} - All contents which can be selected | ||
*/ | ||
function selectableContentList() { | ||
return contentList.value.reduce((newList, content) => { | ||
if (content.kind === ContentNodeKinds.TOPIC && hasCheckbox(content)) { | ||
newList = [...newList, ...content.children.results]; | ||
} else { | ||
newList.push(content); | ||
} | ||
return newList; | ||
}, []); | ||
} | ||
|
||
/** | ||
|
@@ -202,6 +234,9 @@ | |
*/ | ||
function contentPresentInWorkingResourcePool(content) { | ||
const workingResourceIds = workingResourcePool.value.map(wr => wr.id); | ||
if (content.kind === ContentNodeKinds.TOPIC) { | ||
return content.children.results.every(child => workingResourceIds.includes(child.id)); | ||
} | ||
return workingResourceIds.includes(content.id); | ||
} | ||
|
||
|
@@ -226,6 +261,63 @@ | |
}); | ||
} | ||
|
||
const selectAllChecked = computed(() => { | ||
// Returns true if all the resources in the topic are in the working resource pool | ||
const workingResourceIds = workingResourcePool.value.map(wr => wr.id); | ||
const selectableIds = selectableContentList().map(content => content.id); | ||
return selectableIds.every(id => workingResourceIds.includes(id)); | ||
}); | ||
|
||
const selectAllIndeterminate = computed(() => { | ||
// Returns true if some, but not all, of the resources in the topic are in the working | ||
// resource | ||
const workingResourceIds = workingResourcePool.value.map(wr => wr.id); | ||
const selectableIds = selectableContentList().map(content => content.id); | ||
return !selectAllChecked.value && selectableIds.some(id => workingResourceIds.includes(id)); | ||
}); | ||
|
||
const showSelectAll = computed(() => { | ||
return contentList.value.every(content => hasCheckbox(content)); | ||
}); | ||
|
||
function handleSelectAll(isChecked) { | ||
if (isChecked) { | ||
this.addToWorkingResourcePool(selectableContentList()); | ||
} else { | ||
this.contentList.forEach(content => { | ||
var contentToRemove = []; | ||
if (content.kind === ContentNodeKinds.TOPIC) { | ||
contentToRemove = content.children.results; | ||
} else { | ||
contentToRemove.push(content); | ||
} | ||
contentToRemove.forEach(c => { | ||
this.removeFromWorkingResourcePool(c); | ||
}); | ||
}); | ||
} | ||
} | ||
|
||
/** | ||
* @param {Object} param | ||
* @param {ContentNode} param.content | ||
* @param {boolean} param.checked | ||
* @affects workingResourcePool - Adds or removes the content from the workingResourcePool | ||
* When given a topic, it adds or removes all the exercises in the topic from the | ||
* workingResourcePool. This assumes that topics which should not be added are not able to | ||
* be checked and does not do any additional checks. | ||
*/ | ||
function toggleSelected({ content, checked }) { | ||
content = content.kind === ContentNodeKinds.TOPIC ? content.children.results : [content]; | ||
if (checked) { | ||
this.addToWorkingResourcePool(content); | ||
} else { | ||
content.forEach(c => { | ||
this.removeFromWorkingResourcePool(c); | ||
}); | ||
} | ||
} | ||
|
||
const { | ||
hasCheckbox, | ||
topic, | ||
|
@@ -341,6 +433,11 @@ | |
} | ||
|
||
return { | ||
selectAllChecked, | ||
selectAllIndeterminate, | ||
showSelectAll, | ||
handleSelectAll, | ||
toggleSelected, | ||
topic, | ||
topicId, | ||
contentList, | ||
|
@@ -353,6 +450,7 @@ | |
resetWorkingResourcePool, | ||
contentPresentInWorkingResourcePool, | ||
//contentList, | ||
cannotSelectSomeTopicWarning$, | ||
sectionSettings$, | ||
selectFromBookmarks$, | ||
numberOfSelectedBookmarks$, | ||
|
@@ -382,20 +480,6 @@ | |
isTopicIdSet() { | ||
return this.$route.params.topic_id; | ||
}, | ||
isSelectAllChecked() { | ||
// Returns true if all the resources in the topic are in the working resource pool | ||
const workingResourceIds = this.workingResourcePool.map(wr => wr.id); | ||
return this.contentList.every(content => workingResourceIds.includes(content.id)); | ||
}, | ||
selectAllIndeterminate() { | ||
// Returns true if some, but not all, of the resources in the topic are in the working | ||
// resource | ||
const workingResourceIds = this.workingResourcePool.map(wr => wr.id); | ||
return ( | ||
!this.isSelectAllChecked && | ||
this.contentList.some(content => workingResourceIds.includes(content.id)) | ||
); | ||
}, | ||
|
||
getBookmarksLink() { | ||
// Inject the showBookmarks parameter so that | ||
|
@@ -423,6 +507,12 @@ | |
}, | ||
}, | ||
methods: { | ||
showTopicSizeWarningCard(content) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes me think that we need to properly update this method to handle to topic selection- for now I think it's getting lost in the way we are handling back route navigation. |
||
return !this.hasCheckbox(content) && content.kind === ContentNodeKinds.TOPIC; | ||
}, | ||
showTopicSizeWarning() { | ||
return this.contentList.some(this.showTopicSizeWarningCard); | ||
}, | ||
/** @public */ | ||
focusFirstEl() { | ||
this.$refs.textbox.focus(); | ||
|
@@ -446,22 +536,6 @@ | |
|
||
return {}; // or return {} if you prefer an empty object | ||
}, | ||
toggleSelected({ content, checked }) { | ||
if (checked) { | ||
this.addToWorkingResourcePool([content]); | ||
} else { | ||
this.removeFromWorkingResourcePool(content); | ||
} | ||
}, | ||
toggleTopicInWorkingResources(isChecked) { | ||
if (isChecked) { | ||
this.addToWorkingResourcePool(this.contentList); | ||
} else { | ||
this.contentList.forEach(content => { | ||
this.removeFromWorkingResourcePool(content); | ||
}); | ||
} | ||
}, | ||
topicListingLink({ topicId }) { | ||
return this.$router.getRoute( | ||
PageNames.QUIZ_SELECT_RESOURCES, | ||
|
@@ -607,4 +681,9 @@ | |
margin-top: 2em; | ||
} | ||
|
||
.shadow { | ||
box-shadow: 0 1px 3px 0 rgba(0, 0, 0, 0.2), 0 1px 1px 0 rgba(0, 0, 0, 0.14), | ||
0 2px 1px -1px rgba(0, 0, 0, 0.12); | ||
} | ||
|
||
</style> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,7 @@ | |
:isTopic="isTopic" | ||
/> | ||
</div> | ||
<slot name="notice"></slot> | ||
</div> | ||
|
||
</router-link> | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 also want to note that this string is showing at up at the first level when I use back navigation to navigate back . In addition, I tested with QA channel and
tigil-fajod
but one will not show up at the end of the routes when using the back button to exit the navigation except the active working resource pool .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 that once #11862 is merged with this that this will be resolved - I'll make note of this to check it specifically in testing after merging.