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: add autoCommitEdit grid option #725

Merged
merged 1 commit into from
Feb 9, 2023
Merged

feat: add autoCommitEdit grid option #725

merged 1 commit into from
Feb 9, 2023

Conversation

ghiscoding
Copy link
Collaborator

@ghiscoding ghiscoding commented Feb 9, 2023

  • this new option is helpful when autoEdit is enabled, by default the autoEdit will automatically navigate down and automatically open the next row cell, however when autoCommitEdit is enabled it will save but not open the next row cell

differences between autoEdit and the new autoCommitEdit

  • autoEdit after an editor save, will automatically change active cell to the next bottom cell editor and open it
  • autoCommitEdit after an editor save, will stay in the same cell position and will not open the next row editor

Note:
I actually already have this feature in all my other SlickGrid libs since couple years but I was actually patching the issue by calling commitCurrentEdit() (which I also added in this PR) but then it was automatically navigating down, so what I was doing was to change again the cell position to the previous position that the editor originated. So I decided to do this PR to avoid having to patch my libs and fix the source directly. This PR will avoid having to change position twice and fix the source (SlickGrid has always navigated down when autoEdit is enabled but not every user are happy with this).

SlickGrid/slick.grid.js

Lines 5330 to 5332 in b58e4bc

if (options.autoEdit) {
navigateDown();
}

brave_UMgjvjLCrF

- this new option is helpful when `autoEdit` is enabled, by default the `autoEdit` will automatically navigate down and automatically open the next row cell, however when `autoCommitEdit` is enabled it will save but not open the next row cell
@ghiscoding
Copy link
Collaborator Author

ghiscoding commented Feb 10, 2023

@6pac are you still expecting to work on the visible columns? If yes then I can wait for a release but if you're not then I wouldn't mind a release to close all issues related to this PR. I don't have anything else planned after this one.

@6pac
Copy link
Owner

6pac commented Feb 10, 2023

I should have something for you over the weekend. Looking at it, it is more complicated than I at first thought. I'm going to try a simple no-brainer approach first, but I'm not 100% sure it will work with the grid internals.
I'm assuming that if I post it on a branch as a PR then all the tests will run and I'll be able to tell how well it worked. (I know you do that all the time, but I'm still relatively new to the process).
If there are problems, there are a few slightly more complex strategies I can use. But we'll cross that bridge when we come to it.

@ghiscoding
Copy link
Collaborator Author

@6pac yeah you should always use the PR process to make sure all tests passes, you just need to create a branch and push the branch to remote which will allow you to create a PR. The other way to run the tests would be to follow the instructions that I've put in the readme E2E tests with Cypress

@ghiscoding ghiscoding deleted the feat/edit-auto-commit branch February 10, 2023 04:40
@ghiscoding
Copy link
Collaborator Author

@6pac hey just to make sure, you're not going to introduce a breaking change with your visible change, right? I still think adding extra arguments to existing methods, like getColumns(withVisible?: boolean), is the way to go. I probably won't use your new change at first and I would still expect my current implementation to work like it always did.

@6pac
Copy link
Owner

6pac commented Feb 10, 2023

Absolutely. Treat this as experimental for now, I won't merge anything until you fully approve it.

@ghiscoding
Copy link
Collaborator Author

sure, I'll review whenever you push a branch and PR for it ;)

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