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(editor): auto commit before save; add onBeforeEditMode callback #1353

Merged
merged 2 commits into from
Jan 19, 2024
Merged

feat(editor): auto commit before save; add onBeforeEditMode callback #1353

merged 2 commits into from
Jan 19, 2024

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Jan 19, 2024

This addresses the latest review feedback by adding the onBeforeEditMode callback and commiting active changes before running save.

related issue ##1299

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2657d90) 99.5% compared to head (3d28765) 99.5%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1353     +/-   ##
========================================
+ Coverage    99.5%   99.5%   +0.1%     
========================================
  Files         199     199             
  Lines       21571   21574      +3     
  Branches     7203    7203             
========================================
+ Hits        21457   21460      +3     
  Misses        114     114             

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

@ghiscoding
Copy link
Owner

ghiscoding commented Jan 19, 2024

awesome, so I assume the Duration problem that I described earlier is all gone now that you use the editor commit?

@zewa666
Copy link
Contributor Author

zewa666 commented Jan 19, 2024

yep. additionally I've made sure to only call it if the respective save button is the same as the active editor (multirow edit).

hmm seems example16 is failing in e2e tests. will have to check whehter its due to my changes

@ghiscoding
Copy link
Owner

ghiscoding commented Jan 19, 2024

hmm seems example16 is failing in e2e tests. will have to check whehter its due to my changes

nahhh forget about it, it's a flaky Cypress tests, it fails every 20 runs or so. The auto-scroll speed is not entirely consistent, I typically just rerun the test and it passes every single time afterward. I wish that Cypress had an option to rerun the entire spec instead of just it blocks but they don't and it's not enough in that spec file. I know Cypress is pushing users to use scoped test blocks but I still prefer serial tests but sadly Cypress offers less support for that approach.

@zewa666 so unless you have other things in mind, I'm expecting to push a new release tomorrow. I think pretty much covered all small fixes I could get since I'm approaching the end of what I'm able to test in SlickGrid core file (~100 lines left to test, but I'll probably stop soon since they will require too much effort to find how to test them all). It's also nice to see that the amount of Cypress E2E tests also increased quite a lot in this repo, especially since latest v4.x, more tests is always good :)

@ghiscoding ghiscoding changed the title feat: auto commit before save; add onBeforeEditMode callback feat(editor): auto commit before save; add onBeforeEditMode callback Jan 19, 2024
@ghiscoding ghiscoding merged commit f33bf52 into ghiscoding:master Jan 19, 2024
7 checks passed
@ghiscoding
Copy link
Owner

ghiscoding commented Jan 20, 2024

@zewa666 Totally out of topic but didn't want to open a new discussion or anything... I just added a new Column option reorderable to lock certain columns (like checkbox selector for row selection). I added it first in SlickGrid and will add it to Slickgrid-Universal tomorrow before releasing, see this SlickGrid PR, anything to comment feel free to leave a comment in the PR (will merge tomorrow, time for bed now it's 2AM)

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.

2 participants