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

make "ctrl + c" great again #1379

Merged
merged 9 commits into from Feb 8, 2024
Merged

make "ctrl + c" great again #1379

merged 9 commits into from Feb 8, 2024

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Feb 5, 2024

as discussed in #1374, here's my approach to both fix the original issue with the missing row/cell ref as well as the new feature to use the formatter even if an editor is defined or grid is editable but the checked cell doesn't currently contain the active editor

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 5, 2024

I think its best to try out with the modified example 19.

Clicking on a cell, hitting escape and than performing a copy should get you the rendered value of the formatter.
If the editor is open, it should call the editors value serializer (empty string if empty editor, otherwise the entered value)
Also check out selecting a range and copying that to the buffer.

Copy link

codecov bot commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (02c4bc1) 99.8% compared to head (111645c) 99.8%.
Report is 2 commits behind head on master.

❗ Current head 111645c differs from pull request most recent head c7002b5. Consider uploading reports for the commit c7002b5 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1379     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         199     199             
  Lines       21564   21566      +2     
  Branches     6940    6941      +1     
========================================
+ Hits        21501   21503      +2     
  Misses         63      63             

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

@ghiscoding
Copy link
Owner

make "ctrl + c" great again

gosh you make me think about politics in the south of our border, man I dislike that guy so much 😝

I'll have to give a try later after work, but since you're in this subject, it might of interest to review this SlickGrid issue with Undo. I was waiting for a contribution but the guy never replied back and I never investigated (apart from trying and confirming that it's a bug)

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 5, 2024

I can look into the other issue after this PR. I have no idea what that actually is about but we'll find it out ;)

with regards to the title, I'll add a sarcasm note next time :) I really didnt find any suitable one since its both a fix and feat with this PR so plz feel free to update to your preference

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 5, 2024

I really didnt find any suitable one since its both a fix and feat with this PR so plz feel free to update to your preference

hahah no no it's all good, I just wrote what came out of my head at the time of reading lol.

Not sure if you saw my other, but out of subject, post in the discussion. Knowing that you're a fan of Prettier, have you read about Biome? I'm not intending to use it in here but I will for sure use it on new project and dump Prettier (in fact I think I will revisit my other projects using Prettier and migrate them to Biome). Only install Biome and get rid of all Prettier & ESLint deps with just 1 Biome dep, config is easy and it's damn fast

Biome is a fast formatter for JavaScript, TypeScript, JSX, and JSON that scores 97% compatibility with Prettier, saving CI and developer time

Perf 35x faster (written in Rust)

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 5, 2024

ups yeah missed the other one. tbh I wasnt quite a fan of the original tooling Rome as it felt just the wrong way to stuff everything in one tool as oposed to the Unix mentality of a single tool for a single purpose. but ultimately I'm not married to either tool. as long as I can press my autofix shortcut and dont have to manually format I'm all for it, whatever its called 😅

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 5, 2024

I also was not sold at all on the Rome idea and never tried it, but I sometime go on the BunJS GitHub for fun just to see what PRs comes in and saw they had a PR last week migrating from Prettier to Biome and decided to give it a try myself. I like the speed but also the setup of installing just 1 dependency (Biome) and not a ton of ESLint/TypeScript plugins to just do 1 thing at the end of the day style+lint, Biome offers that and I like it so far :)

Going back in time.. I wonder why they ever deprecated TSLint and decided that following ESLint + a ton of TypeScript deps and config was the best approach... really!?! lol

@ghiscoding
Copy link
Owner

@zewa666
I have a question which is totally out of subject from this PR but... do you have access to a Mac/Linux box? I only have Windows computers but I'd like to test that the new rewrite of Excel-Builder-Vanilla that I'm working on is working correctly on other platforms (I've replaced JSZip with fflate, which is ESM, just last night and want to make sure it works on Mac too), if you do then go to the Live Demo on any examples and click the download excel button. Thanks

I worked late on that project last night so I didn't have a chance to try your PR just yet but I'm just about to test it now

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 6, 2024

hmm why would the OS matter with regards to the export? stuff like carriage return line feed? I'll check out on Linux and perhaps get a chance to verify on a mac safari tomorrow at work.

EDIT:
here's the downloaded xlsx within LibreOffice on Ubuntu
2024-02-06_20-41

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 6, 2024

Going back in time.. I wonder why they ever deprecated TSLint and decided that following ESLint + a ton of TypeScript deps and config was the best approach... really!?! lol

eslint already had feature parity at that time with tslint, plus it had a way better plugin infra. last but not least new dev slowed down for tslint due to lack of community. I think the unification was actually good. plus you typically only go once through your setup and leave it at that afterwards, so its a pretty small price for the flexibility and autofix support.

I'm a big fan of being able to tweak even smaller details like separate library imports from local file imports for readability and eslint made lots of these possible

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 6, 2024

yeah the Excel export shouldn't matter but I'm playing it safe...

As for the ESLint subject, yes I know all the reasoning behind the move and all the history and it made some sense to merge the 2, however some tools evolve in a way that requires more setup than before (TSLint was really easy to setup). We can also say the same thing for the JS World evolvement, back in the day it was super easy to work with JS, just add a <script> ref and you're good to go... but nowadays you have install WebPack/Vite/Parcel, import the JS lib and finally start coding, I know the "why" I have to do this but still it was easier back in the day to do quick POC. Anyway, I think you get my point, just thinking out aloud 😝

EDIT

From your print screen, I guess LibreOffice doesn't support gradient background (it might be an MS Excel only feat), no biggy... Just so you know, I tried to make the UI look like the final export. My main concern was really migrating from JSZip to fflate, it's kinda hard to use it and wasn't sure how to create the zip and the compression stuff and just wanted to confirm that it works. Thanks

image

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 6, 2024

Back to the PR... I'm not sure if it's related or not to the previous SlickGrid issue but the Undo (Ctrl+Z) doesn't seem to work at all, I'm pretty sure it was working at some point but I just tried it in Angular-Slickgrid Example 4 and I don't see it working there either. Is it possible that some of our latest changes broke the Undo?
Also is the cell selection supposed to be cancelled when an Editor is open? It looks like I need to close the Editor, via Esc, before I can start selecting again.

msedge_KRcStXtgo5

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 6, 2024

Never saw the undo feature so far on this example. Are you sure it was there? The current master definitely doesn't have it.

The cell selection not working when an Editor is opened was another bug I've noticed before as well which I'll tackle later down the road. Not introduced by any of these changes.


Yeah JS used to be way easier but also way crazier back than. Remember when we had to deal with constructor functions and the revealing module pattern due to lack of visibility modifiers? :)

yup LibreOffice doesn't support the full range of features as does Excel

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 6, 2024

I could be wrong but I think the Undo (Ctrl+Z) came built-in with the plugin, you can see it working in this SlickGrid Example.... ahh nevermind, it requires the user to setup its own Undo by using the clipboardCommandHandler from the plugin, it's very similar to what we do in Example 12 with the undo via the editCommandHandler (notice the handler callback is not the same name). So perhaps we can evolve the Example 19 (or not since it's already large) with it in the future (separate PR), below is the ref for this Undo in SlickGrid example

https://github.com/6pac/SlickGrid/blob/25134186c2c68b24f6f67a2a007b346c72b507e7/examples/example-excel-compatible-spreadsheet.html#L130-L159

.eslintrc Show resolved Hide resolved
@zewa666
Copy link
Contributor Author

zewa666 commented Feb 6, 2024

with regards to undo, I remember we talked about a generic undo/redo plugin which users can easily turn on/off. perhaps thats better handled with that instead of recreating logic everywhere. on top of that I agree that example19 starts to become a bit overloaded.

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 7, 2024

thats very interesting ... the slickCore.ts issue on line 450 isn't picked up locally but throws by the CI ...
oooh ... bc on master it's different and modified to be a string 🤦

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 7, 2024

ah yeah I tweaked the types slightly whenever I see some while reading the code.

Does Husky just run when you create the PR? Do we need to add anything to the CI for that? And is your PR ready for final review before merge?

I'm making good progress with Excel-Builder-Vanilla, I guess I'll be done by the weekend and give it a try in Slickgrid-Universal afterward... can't wait to see the drop in build size 🍰

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 8, 2024

Husky runs on every git push command. no relation to PRs.

my PR is rdy for the final review.

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 8, 2024

The cell selection not working when an Editor is opened was another bug I've noticed before as well which I'll tackle later down the road. Not introduced by any of these changes.

Just reviewing all comments as well, so I assume this will be addressed later

The rest of the PR looks good, I'll go ahead and merge

@ghiscoding ghiscoding merged commit 3bf59e0 into ghiscoding:master Feb 8, 2024
3 checks passed
@ghiscoding
Copy link
Owner

ghiscoding commented Feb 8, 2024

@zewa666 I just realized that the title of this PR doesn't have feat: or fix: prefix so it won't be showing in the changelog unless I add it manually (which I sometime do). Perhaps that something Husky can prevent

@zewa666
Copy link
Contributor Author

zewa666 commented Feb 8, 2024

yup, conventional commiit messages can be enforced with it. I'll set that up

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

2 participants