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

fix: Edit cell mouseout should save & excel copy buffer should still work #917

Merged

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented Feb 23, 2023

  • fixes a regression caused by previous PR fix(common): Excel copy cell ranges shouldn't lose its cell focus #901 and identified in Angular-Slickgrid issue 1103, the previous PR fix(common): Excel copy cell ranges shouldn't lose its cell focus #901 was put in place to fix cell external copy in a bug reported in Slickgrid-React issue 36
  • this bug actually came from a very old patch that was put in place via suppressActiveCellChangeOnEdit: true in SlickGrid PR 243 and this flag was to avoid trigger an event when the active cell changes, this event was being listened by SlickCellSelectionModel and when triggered was then sending another event with the cell ranges that changed, which then sent another event onSelectedRangesChanged which was itself listened by SlickCellExternalCopyManager and when triggered was calling a grid.focus() and when it did that, it was making the editor loses its focus (hence the implementation of suppressActiveCellChangeOnEdit) which is required by SlickCellExternalCopyManager to be able to copy & paste cell ranges. After investigating, I was able to remove the use of suppressActiveCellChangeOnEdit (now disabled globally) by simply calling grid.focus() in each editor prior to itself getting its own focus, so at least our focus remains in the grid and our editor no longer loses its focus and we are also able to finally use Editor + CellExternalCopyManager at the same time without conflict anymore
  • hopefully this fixes all regressions and it also removes some old hacks (suppressActiveCellChangeOnEdit) that was put in place, in summary, it should be a lot cleaner now

msedge_bHvjvC51nu

- fixes a regression caused by previous PR #901 and identified in Angular-Slickgrid issue [1103](ghiscoding/Angular-Slickgrid#1103), the previous PR #901 was put in place to fix cell external copy in a bug reported in Slickgrid-React issue [36](ghiscoding/slickgrid-react#36)
- this bug actually came from a very old patch that was put in place via `suppressActiveCellChangeOnEdit: true` in SlickGrid [PR 243](6pac/SlickGrid#243) and this flag was to avoid trigger an event when the active cell changes, this event was being listened by SlickCellSelectionModel and when triggered was then sending another event with the cell ranges that changed, which then sent another event onSelectedRangesChanged which was itself listened by SlickCellExternalCopyManager and when triggered was calling a `grid.focus()` and when it did that, it was making the editor loses its focus (hence the implementation of `suppressActiveCellChangeOnEdit`) which is required by SlickCellExternalCopyManager to be able to copy & paste cell ranges. After investigating, I was able to remove the use of `suppressActiveCellChangeOnEdit` (now disabled globally) by simply calling `grid.focus()` in each editor prior to itself getting its own focus, so at least our focus remains in the grid and our editor no longer loses its focus and we are also able to finally use Editor + CellExternalCopyManager at the same time without conflict anymore
- hopefully this fixes all regressions and it also removes some old hacks (suppressActiveCellChangeOnEdit) that was put in place, in summary, it should be a lot cleaner now
@ghiscoding ghiscoding changed the title fix: Edt cell mouseout should save & excel copy buffer should still work fix: Edit cell mouseout should save & excel copy buffer should still work Feb 23, 2023
@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Merging #917 (5e32a98) into master (3aa5447) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #917   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          245       245           
  Lines        16598     16608   +10     
  Branches      5907      5906    -1     
=========================================
+ Hits         16598     16608   +10     
Impacted Files Coverage Δ
packages/common/src/services/gridEvent.service.ts 100.00% <ø> (ø)
packages/common/src/editors/autocompleterEditor.ts 100.00% <100.00%> (ø)
packages/common/src/editors/checkboxEditor.ts 100.00% <100.00%> (ø)
packages/common/src/editors/dateEditor.ts 100.00% <100.00%> (ø)
packages/common/src/editors/dualInputEditor.ts 100.00% <100.00%> (ø)
packages/common/src/editors/inputEditor.ts 100.00% <100.00%> (ø)
packages/common/src/editors/longTextEditor.ts 100.00% <100.00%> (ø)
packages/common/src/editors/selectEditor.ts 100.00% <100.00%> (ø)
packages/common/src/editors/sliderEditor.ts 100.00% <100.00%> (ø)
...mon/src/extensions/slickCellExternalCopyManager.ts 100.00% <100.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ghiscoding-SE pushed a commit to ghiscoding/Angular-Slickgrid that referenced this pull request Feb 23, 2023
- fixes #1103 caused by a regression introduced in Slickgrid-Universal PR [901](ghiscoding/slickgrid-universal#901)
- requires Slickgrid-Universal PR [917](ghiscoding/slickgrid-universal#917) to be merged and released
- the regression came after I wanted to fix another bug which was that making a cell range and Copy+Paste wasn't working, when fixing that bug it caused a new bug (this regression). This PR should fix both of these bugs and remove a very old hack that was introduced with `suppressActiveCellChangeOnEdit` which is no longer required
- added Cypress E2E tests to cover the bug identified in #1103
@ghiscoding ghiscoding merged commit 18ba0fc into master Feb 23, 2023
@ghiscoding ghiscoding deleted the bugfix/remove-usage-of-suppressActiveCellChangeOnEdit branch February 23, 2023 22:30
ghiscoding added a commit to ghiscoding/aurelia-slickgrid that referenced this pull request Feb 23, 2023
- fixes a bug identified in Angular-Slickgrid [issue](ghiscoding/Angular-Slickgrid#1103) caused by a regression introduced in Slickgrid-Universal PR [901](ghiscoding/slickgrid-universal#901)
- requires Slickgrid-Universal PR [917](ghiscoding/slickgrid-universal#917) to be merged and released
- the regression came after I wanted to fix another bug which was that making a cell range and Copy+Paste wasn't working, when fixing that bug it caused a new bug (this regression). This PR should fix both of these bugs and remove a very old hack that was introduced with `suppressActiveCellChangeOnEdit` which is no longer required
- added Cypress E2E tests to cover the bug identified in #1103
ghiscoding added a commit to ghiscoding/aurelia-slickgrid that referenced this pull request Feb 24, 2023
…rk (#941)

- fixes a bug identified in Angular-Slickgrid [issue](ghiscoding/Angular-Slickgrid#1103) caused by a regression introduced in Slickgrid-Universal PR [901](ghiscoding/slickgrid-universal#901)
- requires Slickgrid-Universal PR [917](ghiscoding/slickgrid-universal#917) to be merged and released
- the regression came after I wanted to fix another bug which was that making a cell range and Copy+Paste wasn't working, when fixing that bug it caused a new bug (this regression). This PR should fix both of these bugs and remove a very old hack that was introduced with `suppressActiveCellChangeOnEdit` which is no longer required
- added Cypress E2E tests to cover the bug identified in #1103
ghiscoding added a commit to ghiscoding/slickgrid-react that referenced this pull request Feb 24, 2023
- fixes a bug identified in Angular-Slickgrid [issue](ghiscoding/Angular-Slickgrid#1103) caused by a regression introduced in Slickgrid-Universal PR [901](ghiscoding/slickgrid-universal#901)
- requires Slickgrid-Universal PR [917](ghiscoding/slickgrid-universal#917) to be merged and released
- the regression came after I wanted to fix another bug which was that making a cell range and Copy+Paste wasn't working, when fixing that bug it caused a new bug (this regression). This PR should fix both of these bugs and remove a very old hack that was introduced with `suppressActiveCellChangeOnEdit` which is no longer required
- added Cypress E2E tests to cover the bug identified in Angular-Slickgrid
ghiscoding added a commit to ghiscoding/slickgrid-react that referenced this pull request Feb 24, 2023
…rk (#57)

- fixes a bug identified in Angular-Slickgrid [issue](ghiscoding/Angular-Slickgrid#1103) caused by a regression introduced in Slickgrid-Universal PR [901](ghiscoding/slickgrid-universal#901)
- requires Slickgrid-Universal PR [917](ghiscoding/slickgrid-universal#917) to be merged and released
- the regression came after I wanted to fix another bug which was that making a cell range and Copy+Paste wasn't working, when fixing that bug it caused a new bug (this regression). This PR should fix both of these bugs and remove a very old hack that was introduced with `suppressActiveCellChangeOnEdit` which is no longer required
- added Cypress E2E tests to cover the bug identified in Angular-Slickgrid
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.

None yet

1 participant