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

feat(dataView): add option to apply row selection to all pages #716

Merged
merged 10 commits into from
Feb 3, 2023

Conversation

ghiscoding
Copy link
Collaborator

@ghiscoding ghiscoding commented Dec 27, 2022

  • supersede WIP - fix(dataView): add option to apply row selection to all pages #689, most Grid & DataView changes were kept with some small changes & fixes, also added more Cypress E2E tests
  • when having pages and syncGridSelection is called with preserveHiddenOnSelectionChange, clicking on Select All checkbox:
    • it will apply row selections on all rows
    • if nothing is selected and we filter data, then we click on Select All, it will apply row selections only to the item being filtered and if we remove filters then the Select All is not expect to be selected

Changes compared to previous PR #689

  • most of the files are the same (slick.grid.js, slick.checkboxselectcolumn.js, example-checkbox-header-row.html), however slick.dataview.js got modified a bit
  • added back filteredIds which was added originally but removed in PR WIP - fix(dataView): add option to apply row selection to all pages #689
  • the method syncGridSelection had a onSelectedRowIdsChanged.subscribe (1) inside it but there was a racing issue because the Example (demo) is also calling onSelectedRowIdsChanged (2), but the sequence should be run (1) and then only after run (2), we could fix this by adding a setTimeout but it isn't a very clean approach, however we can simply change the (1) from onSelectedRowIdsChanged.subscribe to a function named selectedRowChangedFn and execute it prior to (2), this way is a clean approach and it doesn't have any racing issue since we are only left with the (2) subscribe
  • I still don't understand the difference between preserveHidden and preserveHiddenOnSelectionChange but as far as I see it, the Example only works as intended (the steps written above) when these 2 flags are set to preserveHidden: false and preserveHiddenOnSelectionChange: true. I'm not sure if this is intended @arashdalir could probably confirm
  • this PR supersede WIP - fix(dataView): add option to apply row selection to all pages #689 and is based on SlickGrid v3.x
  • I don't think the changes to slick.grid.js are required for applying Select All across all pages, we should probably move that code change to a separate PR

Requires Confirmations before merging

It requires review & confirmation by @arashdalir before merging this PR

- supersede #689, most Grid & DataView changes were kept with some small changes & fixes, also added more Cypress E2E tests
- when having pages and `syncGridSelection` is called with `preserveHiddenOnSelectionChange`,  clicking on Select All checkbox:
  - it will apply row selections on all rows
  - if nothing is selected and we filter data, then we click on Select All, it will apply row selections only to the item being filtered and if we remove filters then the Select All  is not expect to be selected
slick.dataview.js Outdated Show resolved Hide resolved
@ghiscoding ghiscoding changed the title feat(dataView): add option to apply row selection to all pages WIP - feat(dataView): add option to apply row selection to all pages Dec 27, 2022
@ghiscoding
Copy link
Collaborator Author

brave_BODonh5JQw

@ghiscoding ghiscoding changed the title WIP - feat(dataView): add option to apply row selection to all pages feat(dataView): add option to apply row selection to all pages Feb 2, 2023
@ghiscoding
Copy link
Collaborator Author

ghiscoding commented Feb 2, 2023

@6pac it doesn't look like I'll get a review from that other person since he doesn't seem to be around. This PR is been pending for over a month and I'm finished testing it in my libs and fixed all issues I found.

So can we merge it?
Note that I'm not ready for a new release just yet, I have 2 more PRs to do after this one. Thanks

@arashdalir
Copy link
Contributor

arashdalir commented Feb 2, 2023 via email

@ghiscoding
Copy link
Collaborator Author

@arashdalir
That's ok, I want to proceed with this change, you might have to adjust some property name when you merge it in your grid but it should be easy enough, the implementation remains the same with the adjustment you provided.

@6pac
let's merge it

@ghiscoding ghiscoding requested a review from 6pac February 2, 2023 16:56
@6pac 6pac merged commit 6e4e83a into master Feb 3, 2023
@ghiscoding ghiscoding deleted the feat/dataview-apply-row-selections-all-pages branch February 10, 2023 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants