-
-
Notifications
You must be signed in to change notification settings - Fork 24
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 for page being cleared when using copy and paste with selectEditor #836
Fix for page being cleared when using copy and paste with selectEditor #836
Conversation
…t.body. This fixes an issue with the page being cleared when using copy/paste with the single select editor.
Codecov Report
@@ Coverage Diff @@
## master #836 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 239 239
Lines 16365 16369 +4
Branches 5777 5777
=========================================
+ Hits 16365 16369 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
hmm I'll have to try that locally but I'm wondering if this will leave an orphan div in the DOM. Perhaps we could create the div outside of the object and then destroy it after we're done with it. Can you try with something like this + const tmpDiv = document.createElement('div');
const editor = new (columnDef as any).editor({
- container: document.createElement('div'), // a dummy container
+ container: tmpDiv, // a dummy container
column: columnDef,
position: { top: 0, left: 0 }, // a dummy position required by some editors
grid: this._grid
});
// ...
editor.destroy();
+ tmpDiv.remove(); |
I was wondering about that as well. I just assumed it wouldn't actually get added to the DOM since we don't append it to anything. I also assumed the javascript object would just get garbage collected. I'll make the changes now. There's some similar code on line 113 in the same file. I'll make the changes to that dummy container as well. |
I looked at the original plugin from SlickGrid and they were using https://github.com/6pac/SlickGrid/blob/master/plugins/slick.cellexternalcopymanager.js#L120 I see the Cypress tests failed but it's a little flaky sometime, it seems like a flaky test so I restarted the job, it shouldn't be related to your PR. I don't think I have any tests for any copy & paste stuff. Have you at least tried it locally with your changes? |
Yes, I tried it locally with my changes and it fixed the issue. I ran the unit tests, and I just kicked off the Cypress tests locally. |
ok thanks, I'll give it a try later tonight after work. I also have 1 more thing to look into before doing a new patch release, I should be able to release by the end of the week with your fix and others commits. Thanks for the contribution |
Awesome, you're welcome. Thank you for maintaining the whole project! |
@austinsimpson Oh I just realized your PR title didn't follow Conventional Commits, please make sure to follow that on the next time around so that your commit(s) appears automatically in releases. I'll have to remember to add it manually to the changelog, automation are better than my memory 😉 |
Thanks for linking that, I will read it. I'm using slickgrid-angular. I was able to reproduce the issue by cloning the angular examples repo. I was running the bootstrap 5 examples. I edited the editors example in grid-editor.component.ts:164 and changed the Title2 column to use the following configuration.
When I copy and paste "Task 1" I get the blank screen issue. I can get a screen recording if it's helpful. |
Ok good to know, hopefully that fixes your issue. I made another commit to add a note that this option doesn't work with Row Move & Row Selection, these plugins conflict with each other because they both use dragging and requires to change some styling on the selected cell to copy and that conflicts with row selection because it also does apply styling on the rows. I tried to have as much Examples as possible in this lib to reflect all the features shown in Angular-Slickgrid but we could also add more Examples in here too if certain features are hard to test and troubleshoot. |
@austinsimpson thanks again for the contribution, this is now available in the release of Angular-Slickgrid v5.1.3 and Slickgrid-Universal v2.1.3, there are few other fixes which you can see in each release. |
@ghiscoding you're welcome. Thank you for getting this out! |
I encountered an issue using copy and paste with the selectEditor control. When data is pasted into a column that uses selectEditor, the page will be cleared. This is because the main branch is using document.body as the container for the element when setting the value for the column.
Here is the line of code that creates the editor with document.body as the container:
slickgrid-universal/packages/common/src/extensions/slickCellExternalCopyManager.ts
Line 139 in 76e9719
We can see the line that clears the container in selectEditor.ts:
slickgrid-universal/packages/common/src/editors/selectEditor.ts
Line 738 in 76e9719
I was able to fix this by using a new div as the container instead.