-
Notifications
You must be signed in to change notification settings - Fork 28
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 'select' method to allow deselecting selection #129
base: main
Are you sure you want to change the base?
Conversation
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 see that you copied select method from selectionManager and this is correct, but I'm not sure if we should copy spaghetti code, what do you think?
resolve(this.selectionIds); | ||
}) as any; | ||
} | ||
|
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 see a lot of If else
blocks and all of them have the same this.selectionIds = selectionIds
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.
If this is right, you can make it much easier:
if(multiSelect){
...all your ifs here
} else {
this.selectionIds = selectionIds
}
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 refactored the code to make it more readable.
src/mocks/mockISelectionManager.ts
Outdated
// if no multiselect reset current selection and save new passed selections; | ||
if (!multiSelect) { | ||
this.selectionIds = selectionIds; | ||
if (selectionIds.length < 1) { |
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.
selectionIds.length === 0
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.
Fixed.
src/mocks/mockISelectionManager.ts
Outdated
@@ -112,6 +133,8 @@ export class MockISelectionManager implements ISelectionManager { | |||
} | |||
|
|||
public simutateSelection(selections: ISelectionId[]): void { | |||
this.callback(selections); | |||
if (this.callback && typeof this.callback === "function") { |
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.
type guard is enough here, it also checks for undefined
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.
Fixed.
|
||
const selectedIds: ISelectionId[] = selectionManager.getSelectionIds(); | ||
expect(selectedIds).toHaveSize(3); | ||
expect(selectedIds[0].equals(selectionId0)).toBeTruthy(); |
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.
expect(selectedIds[0]).toEqual(selectionId0)
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.
Fixed.
const selectionId0: ISelectionId = createSelectionId("0"); | ||
const selectionId1: ISelectionId = createSelectionId("1"); | ||
const selectionId2: ISelectionId = createSelectionId("2"); |
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.
You can create a variable with these selectionIds and then compare it with the result using toEqual
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.
Fixed.
CHANGELOG.md
Outdated
@@ -1,3 +1,7 @@ | |||
## 6.1.2 | |||
* Fix 'select' method to allow deselecting selection |
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.
Fix 'select' method in MockISelectionManager to allow deselecting selection
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.
Fixed.
src/mocks/mockISelectionManager.ts
Outdated
this.selectionIds = this.selectionIds.filter(x => !selectionIds[0].equals(x)); | ||
} else { | ||
// if multiSelect is off, the selected item is the new selectedId, else deselect the selection | ||
this.selectionIds = selectionIds.length > 1 ? selectionIds : []; |
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.
Here would be always an empty array
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.
You're right, it's the expected behavior too. Fixed.
…handling and update tests for consistency
Please, view the source code for 'select' method.