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.

Sign up for GitHub

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

Jump to bottom

Fix save quick edit modals #4742

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Fix save quick edit modals #4742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f10ab0
Fix save quick edit modals
AlexVelezLl Sep 18, 2024
6c3e03b
fix linting issues
AlexVelezLl Sep 18, 2024
ea91300
Add tests for mixed options in EditBooleanMapModal
AlexVelezLl Sep 18, 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
6 changes: 4 additions & 2 deletions ...on/contentcuration/frontend/channelEdit/components/QuickEditModal/EditBooleanMapModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,9 @@
},
canSave() {
if (this.hasMixedCategories) {
return Object.values(this.selectedValues).some(value => value.length > 0);
return Object.values(this.selectedValues).some(
value => value.length === this.nodes.length
Copy link
Member

Choose a reason for hiding this comment

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

The updated condition now ensures saving is only allowed when the selected values cover all nodes , which makes validation more robust. What would happen if partial selections are ever needed in other scenarios? maybe we might want to consider making this condition more flexible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey! The thing is that "partial selections" just only serve to determine if we have mixedCategories, but the user doesnt see nor interact with any of these partial selections. For them they dont exists, so thats why we cant depend on them, and rather depend on what they are seeing which is selected options, and we have selected options when we have that value.length === this.nodes.length.

If we keep the condition of value.length > 0 then we will be taking these mixed options as valid values. So if I have for example 2 nodes:

Node 1:

  • Category A selected

Node 2:

  • Category B selected

Then when we open the modal, we will see the mixed options comment, but we wont see any of the options selected, because we dont have any option selected across all nodes, and we wont see any indeterminate state as they dont exist anymore, the only the user will se will be all checkboxes unchecked. But the user will still see the save button enabled, because canSave = true because all category A and B have value.length > 0, and this is an error, as we are not meeting the 4th acceptance criteria mentioned in #4651:

the "save" button only be active if one or more boxes is checked, but no validation is required (and no error message)

So, no, we cant have a more flexible condition as this condition does not met the requirements.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for explaining the reasoning behind the stricter condition.

);
}
return !this.error;
},
Expand Down Expand Up @@ -174,7 +176,7 @@
Object.assign(fieldValue, currentNode[this.field] || {});
}
Object.entries(this.selectedValues)
.filter(([value]) => value.length === this.nodeIds.length)
.filter(entry => entry[1].length === this.nodeIds.length)
Copy link
Member

Choose a reason for hiding this comment

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

Great catch! On correcting the filter. Changing the check to entry[1].length === this.nodeIds.length ensures we're filtering the selected values properly, as the second element of the entries represents the value, not the first.

.forEach(([key]) => {
fieldValue[key] = true;
});
Expand Down
Loading