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

number of resources updates on selection #11825

Conversation

AllanOXDi
Copy link
Member

@AllanOXDi AllanOXDi commented Jan 31, 2024

Summary

References

#11784

Reviewer guidance

  • Verify that the ancestor counts are correctly updated within the selected topic.
  • Confirm that the "Select All" Checkbox counts the total number of resources within the topic tree.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: very small labels Jan 31, 2024
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Key changes that are needed for merge:

  • The first number of the "x of y resources selected" string needs to be updated per my comments
  • Clean up unused commented code
  • Remove side-effects code from the isSelectAllChecked computed property

One thing that is not asked for in the original issue but that I've noted below which would be helpful here is:

  • Update the behavior when the user is unselecting-all so that instead of wiping the user's changes, we just remove the selectable items in the content list from the working resource pool

// },
selectionMetadata(content) {
if (content.kind === ContentNodeKinds.TOPIC) {
const count = this.workingResourcePool.length;
Copy link
Member

Choose a reason for hiding this comment

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

This is making it so that every card shows the same # of resources selected out of however many children there are. For example, the QA Channel's QTI folder has 3 resources in it, but when I select 4 resources from the CK12 folder the QTI card says "4 of 3 resources selected"

This should instead be using the ancestor(s) property on the items in the workingResourcePool where basically count should be "the number of items in the working resource pool who has this topic as an ancestor".

Comment on lines 297 to 298
// selectionMetadata(content) {
// console.log(content);
Copy link
Member

Choose a reason for hiding this comment

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

All of this commented out stuff can be removed

Comment on lines 277 to 286
if (this.contentList.every(content => workingResourceIds.includes(content.id))) {
this.contentList.forEach(resource => {
if (resource.kind === ContentNodeKinds.TOPIC) {
this.addToWorkingResourcePool(resource.children.results);
}
});
return true;
} else {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Lines 278-282 here seem out of place. Since isSelectAllChecked is supposed to just be a Boolean computed property, it can get really hairy if we introduce a side effect here like adding items to the resource pool.

This logic probably belongs below in the method we're passing as the handler function for the @changeselectall event on the ContentCardList. I'll add some more thoughts there.


if (this.contentList.every(content => workingResourceIds.includes(content.id))) {
this.contentList.forEach(resource => {
if (resource.kind === ContentNodeKinds.TOPIC) {
Copy link
Member

Choose a reason for hiding this comment

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

I think that maybe this logic is altogether not necessary here. It seems like you're aiming to handle the user's "select all" click while also including all of the folders' children in that "Select all".

The logic for "selecting folders" can be handled in a separate issue, so this should be reverted to the return this.contentList.every(...).

One thing to bear in mind is that even when we do allow users to select folders, the only time we'll place all of the topic's children into the working resource list is if that folder meets the criteria for having a checkbox in the first place as defined in #11790

this.addToWorkingResourcePool(this.contentList);
} else {
this.isSelectAllChecked = false;
Copy link
Member

Choose a reason for hiding this comment

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

Removing the setting of isSelectAllChecked here is good as it's a computed property.

In my note on that isSelectAllChecked property I mentioned that the side-effects of manipulating the working resource pool should probably be moved here.

In the else block here, however, rather than resetWorkingResourcePool this should instead remove all items in the content list from the working resource pool. resetWorkingResourcePool just undoes all changes the user has made since opening the panel, which is not desired here.

In this case, you'd need to do a this.contentList.forEach(content => this.removeFromWorkingResourcePool(content)) (or something along those lines -- you'll probably also want to be sure to filter out Topics as we aren't permitting full topic selection until #11790 is implemented)

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Changes here look good thanks @AllanOXDi - let's find a time to jump on a call to clean up the conflicts so we can get it merged!

@AllanOXDi AllanOXDi force-pushed the display_number_of_selected_resources branch 2 times, most recently from 72feca2 to 9e9cc2b Compare February 8, 2024 20:25
@AllanOXDi AllanOXDi force-pushed the display_number_of_selected_resources branch from b0865cb to ffee918 Compare February 9, 2024 16:22
@AllanOXDi AllanOXDi merged commit 7dbfdea into learningequality:develop Feb 9, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium SIZE: small SIZE: very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quiz Creation Select Resources - Display proper count & ancestor counts on the card.
2 participants