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(plugins): add ways to use all 3 plugins together, closes #347 #474

Merged
merged 7 commits into from
Mar 27, 2020

Conversation

ghiscoding
Copy link
Collaborator

@ghiscoding ghiscoding commented Mar 21, 2020

  • closes Using Checkboxes with DetailView #347

  • this PR allows to use 3 plugins (RowDetail, RowMoveManager & CheckboxSelector) all at the same time. Our new project requires at least 2 of the 3 to be used together.

  • in order to achieve that, I added 2 new options to the Row Move Manager Plugin, the new options are (singleRowMove & disableRowSelection) and their default are set to false so it doesn't break anyone's code

  • the main changes are done in the RowMoveManager plugin, while a couple of changes were also made in the RowDetail Plugin so that all 3 plugins all work nicely together. A new Example was added as well to demo all 3 plugins in 1 grid, Cypress E2E were also added

  • add a way to use all 3 plugins at the same time

  • add usabilityOverride to skip move icon displayed on each row for the RowMoveManager Plugin (same override as all other plugins)

  • add getColumnDefinition() method in the RowMoveManager Plugin with a custom formatter that is connected to the new usabilityOverride

    • should still work when user provide their own column definition (current behavior)
  • add grid object to the args parameter of onMoveRows and onBeforeMoveRows

  • add Cypress E2E tests

See animated gif below for a demo

iAwSJhB685

demo with usabilityOverride (RowMoveManager Plugin) to make only second rows moveable

image

- this PR allows to use 3 plugins (RowDetail, RowMoveManager & CheckboxSelector) all at the same time
- in order to achieve that, I added 2 new options to the Row Move Manager Plugin, the new options are (singleRowMove & disableRowSelection) and their default are set to false so it doesn't break anyone's code
@ghiscoding ghiscoding changed the title WIP - feat(plugins): add ways to use all 3 plugins together WIP - feat(plugins): add ways to use all 3 plugins together, closes #347 Mar 21, 2020
@ghiscoding ghiscoding changed the title WIP - feat(plugins): add ways to use all 3 plugins together, closes #347 feat(plugins): add ways to use all 3 plugins together, closes #347 Mar 27, 2020
@ghiscoding
Copy link
Collaborator Author

ghiscoding commented Mar 27, 2020

@6pac
I'm done with this PR, there should be enough Cypress E2E to cover the behaviours.
The explanation of the changes are mentioned on top, if you have any questions, feel free to ask.

Since there are a lot of commits, I strongly suggest you use the green "Squash and merge" button when merging. Would you mind doing another release, I'd like to pull the changes through NPM, thanks again mate (take your time though, it can wait a few days or a week, no rush).

@6pac 6pac merged commit 90ead54 into master Mar 27, 2020
@ghiscoding
Copy link
Collaborator Author

ghiscoding commented Mar 27, 2020

@6pac
I saw that you went ahead quite fast on the release, that was awesomely fast, however there seems to be a slight problem as it still seems to point to the previous code. I also went and opened the package.json and it's still referring to 2.4.19. I also did a delete of the package in node_modules but no luck, it still pulls previous code version. BTW, It's from NPM which is where I always get the version when I want to use it. Was it too fast by any chance? ;)

@6pac
Copy link
Owner

6pac commented Mar 27, 2020

It's all 100% automated and worked flawlessly until recently. It seems I'm getting write or permissions failures somewhere in the process.
Dropbox stopped working a while back, I think Windows 10 has become unstable, and I need to reinstall :-(
I'll manually run this through again...but it will have to be this evening.

@ghiscoding
Copy link
Collaborator Author

Perhaps you forgot to update your local code before running npm publish? If so, we might need another version

Ahh ok you automate that process, that is about the only thing I still do manually hehe
Take your time, I'm going to bed anyway, it's past midnight here cheers mate

@6pac
Copy link
Owner

6pac commented Mar 27, 2020

hmmm. .. is it the new package.lock file that's the problem? I may not have integrated that into my processes yet. Everything else looks squeaky clean.

@ghiscoding ghiscoding deleted the feat/example-row-detail-selector branch March 27, 2020 13:20
@ghiscoding
Copy link
Collaborator Author

@6pac
No it's not related since the package.lock is for what the lib needs to download to work while the NPM bundle is what the lib export. For unknown reason it looks like the content of the NPM bundle still has the old bundle content and even the package.json shows 2.4.19 even though on the NPM website it does show 2.4.20, there seems to be some lynching issues somewhere. I also tried on computer from work and I get the same result, still the old files are being downloaded.

BTW, you can test it yourself too if you wish, just npm install slickgrid and a new folder and you'll see the content is not correct

So I think the best is to repackage (or rebundle) it and do another release.

@ghiscoding
Copy link
Collaborator Author

ghiscoding commented Mar 27, 2020

@6pac
🤦‍♂ OMG I'm so sorry... I found the error is on my side, I had 2 repos in the same lib and both had their own package.json but they were NOT at the same version number... so sorry mate. I'm still new to monorepo (multiple packages into 1 repo) and that was completely an error on my side. For reference here is my new project, which is a monorepo.

I found this website Unpkg, it shows exactly what each version of an NPM bundle has, for example unpkg - slickgrid@2.4.21. This helped me find out that the error was completely on my side.

@6pac
Copy link
Owner

6pac commented Mar 28, 2020

no worries, I had just reached the same conclusion. When I npm installed, everything was good.

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.

Using Checkboxes with DetailView
2 participants