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

Add selectionMatchFunction option #824

Merged
merged 4 commits into from
Jan 26, 2021

Conversation

kpfefferle
Copy link
Member

This PR adds an optional selectionMatchFunction to the ember-tbody component. When set, this function will be used in place of === when checking to see if a row should be marked as selected.

Fixes #823 (at least offers a workaround)

@xomaczar
Copy link

xomaczar commented Sep 1, 2020

@mixonic what's blocking this to be merged? missing tests?

@xomaczar
Copy link

xomaczar commented Sep 1, 2020

Never mind I’m not sure what this PR actually does

@kpfefferle kpfefferle force-pushed the kpfefferle/selection-match-function branch from f19e0aa to edd5087 Compare October 22, 2020 19:16
@kpfefferle kpfefferle changed the base branch from master to 3.0-beta October 22, 2020 19:16
@kpfefferle kpfefferle force-pushed the kpfefferle/selection-match-function branch from edd5087 to 6a3ffb7 Compare October 22, 2020 19:21
@kpfefferle kpfefferle requested a review from a team October 22, 2020 19:22
@kpfefferle kpfefferle force-pushed the kpfefferle/selection-match-function branch from 6a3ffb7 to e516eb8 Compare October 22, 2020 20:16
Copy link
Contributor

@twokul twokul left a comment

Choose a reason for hiding this comment

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

👍 A couple of thoughts:

  • can we add a test for this change?
  • should we consider updating the docs to reflect this new API?

@kpfefferle kpfefferle force-pushed the kpfefferle/selection-match-function branch from e516eb8 to efeffb8 Compare December 4, 2020 20:38
Comment on lines +97 to +102
/**
A function that will override how selection is compared to row value.

@argument selectionMatchFunction
@type function?
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

This gets displayed in the docs for the ember-tbody component:

Docs

@kpfefferle
Copy link
Member Author

@twokul I added a test. For docs, see above how the JSDoc comment gets displayed on the docs website. I'm not sure that this function really warrants a full section in the examples.

@kpfefferle kpfefferle force-pushed the kpfefferle/selection-match-function branch from efeffb8 to 7696f96 Compare December 4, 2020 20:42
@kpfefferle kpfefferle requested a review from twokul December 4, 2020 20:43
Copy link
Contributor

@twokul twokul left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Provisional ✅ (pending tests)

Comment on lines +67 to +84
isGroupSelected: computed(
'_tree.{selection.[],selectionMatchFunction}',
'_parentMeta.isSelected',
function() {
let rowValue = get(this, '_rowValue');
let selection = get(this, '_tree.selection');
let selectionMatchFunction = get(this, '_tree.selectionMatchFunction');

isGroupSelected: computed('_tree.selection.[]', '_parentMeta.isSelected', function() {
let rowValue = get(this, '_rowValue');
let selection = get(this, '_tree.selection');
if (!selection || !isArray(selection)) {
return false;
}

if (!selection || !isArray(selection)) {
return false;
let isSelectionMatch = selectionMatchFunction
? selection.filter(item => selectionMatchFunction(item, rowValue)).length > 0
: selection.includes(rowValue);
return isSelectionMatch || get(this, '_parentMeta.isGroupSelected');
}

return selection.includes(rowValue) || get(this, '_parentMeta.isGroupSelected');
}),
),
Copy link
Member Author

Choose a reason for hiding this comment

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

@twokul This set of changes is new and could probably use a final 👀

Adding the tests made me realize that the isGroupSelected CP also needed to use the selectionMatchFunction when matching selection against rows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Additional tests have been added to ensure that selectionMatchFunction works in both single and multiple modes

@kpfefferle kpfefferle requested a review from twokul December 7, 2020 21:06
@mixonic
Copy link
Member

mixonic commented Jan 10, 2021

@kpfefferle did you get to follow up on the tests you wanted to add here?

@kpfefferle
Copy link
Member Author

@mixonic I have not yet done that and it fell off my radar. I'll try to carve out some time to get back to that this week.

@kpfefferle kpfefferle force-pushed the kpfefferle/selection-match-function branch 2 times, most recently from 78f58cd to c1f8391 Compare January 20, 2021 22:05
@kpfefferle
Copy link
Member Author

@mixonic @twokul Additional tests have been added to ensure we're covering both single and multiple selection modes. Ready for review!

@kpfefferle kpfefferle requested a review from a team January 20, 2021 22:08
@kpfefferle kpfefferle force-pushed the kpfefferle/selection-match-function branch from c1f8391 to 4b29a50 Compare January 20, 2021 22:21
@kpfefferle kpfefferle force-pushed the kpfefferle/selection-match-function branch from 4b29a50 to 37cdfcc Compare January 25, 2021 21:22
@twokul
Copy link
Contributor

twokul commented Jan 26, 2021

@kpfefferle Thanks! Merging this.

@twokul twokul merged commit d915544 into 3.0-beta Jan 26, 2021
@twokul twokul deleted the kpfefferle/selection-match-function branch January 26, 2021 20:18
@aemc
Copy link

aemc commented Mar 12, 2022

an example would be nice for this func

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Programatic row selection seems broken?
5 participants