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: paste multiline content as single cell #1563

Merged
merged 6 commits into from
Jun 15, 2024

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Jun 8, 2024

this changes the default behavior to paste multiline strings (formatted with quotes e.g by Excel) as a single cell as opposed to parsing it's content and performing multi-row pastes.

Additionally it adds an option to remove the quotes after paste as well as treat newlines with alternative characters.

fixes #1559

Copy link

stackblitz bot commented Jun 8, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 8, 2024

there are still some Cypress tests missing as well as a couple of docs.

@zewa666 zewa666 marked this pull request as ready for review June 8, 2024 19:48
Copy link

codecov bot commented Jun 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (025888d) to head (6d87978).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1563     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         198     198             
  Lines       21746   21764     +18     
  Branches     7282    7148    -134     
========================================
+ Hits        21685   21703     +18     
  Misses         55      55             
  Partials        6       6             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 11, 2024

so I just googled SlickGrid to see if any recent articles came out and I'm just speechless at this article, nearly all of it is wrong, I'm just like wow this is so wrong... I had to share with someone this article 🤣

There's no mention of any the Migration Guides I wrote, neither the Angular Update website and jumping so many versions in a single go is never recommended and finally most info of what to change are just so wrong.. reinstalling SlickGrid when I got rid of it... just wow! 🤦🏻‍♂️ oh and of course there's no "reply" button of any kind so there's no way to correct them either that's too bad :(


apart from that, I pushed a couple of fixes and I think I'm done with everything, I'll wait for your PR (is this the last one?) and probably push next weekend again.

It's now kind of hard to believe but I think I'm done with the project. I don't have any new features planned. 🚀

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 11, 2024

Its AI week for me. working with Semantic Kernel and building bots. so Grid is on hold but I plan to finish this one by the weekend.

first round of user tests came in and brought some good insights plus a bug report, which I'll create together with a PR.

Re article, it definitely wasnt too insightful but neither it felt offensive. as such I'd just take it easy ;)

having no new features is a good thing. should give us some room to even further harden the current code base and iron out some uncovered bugs.

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 14, 2024

For the love of testing ... I can't figure out how to copy and paste properly with Cypress. Essentially I wanted to test in the second test stub that if you paste the same as in the previous but this time quoted with double quotes, it should stay in one column.

Other than that, the PR is ready to go

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 14, 2024

Yeah some things are harder than needs to be in Cypress, just googling it, have you tried something similar to this SO Answer or the one following it?
https://stackoverflow.com/a/68871590/1212166

For some of the Cypress tests I used the console to log certain things and inspect it (I mainly used it to test Grid State), so that might work too!? The only downside that I had with this approach is that Mocha (which is what Cypress use) doesn't have any function to check if an object contain 1 property that Jest has (so Cypress/Moch calledWith is much more limited), so I had to compare for the entire object equality and it's hard in some cases.

cy.window().then((win) => {
expect(win.console.log).to.have.callCount(1);
expect(win.console.log).to.be.calledWith('Client sample, Grid State changed:: ', { newValues: { pageNumber: 3, pageSize: 20 }, type: 'pagination' });
});

But anyway if you think it's too complicate and we don't need these tests, I'm happy to merge as it is (at least we have the unit tests)

.should('have.text', 'with newlines');
});

it('should keep quoted content on the same cell', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

probably better to remove this empty test if you can't add anything to test underneath ;)

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 15, 2024

I'll give it one more try tonight and otherwise remove the tests and finish the PR

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 15, 2024

nope, no chance. It's the mix of paste plus keydown listener that can't be simulated properly. So whatever, Unit tests will have to suffice here.

Rdy for merge from my side

@ghiscoding
Copy link
Owner

okie dokie, I'm working on a new Example for Footer Totals Row and I'll done. I assume that you already pushed everything you wanted? I'll push a release by next week probably

@ghiscoding ghiscoding merged commit 4398f1d into ghiscoding:master Jun 15, 2024
8 checks passed
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.

Quotes newlines/tabs excelcopybuffer
2 participants