feat: notify onValidationError on paste if validation failed #1462
Conversation
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1462 +/- ##
========================================
+ Coverage 99.8% 99.8% +0.1%
========================================
Files 199 199
Lines 21703 21707 +4
Branches 6992 6993 +1
========================================
+ Hits 21639 21643 +4
Misses 64 64 ☔ View full report in Codecov by Sentry. |
| @@ -127,15 +127,15 @@ export class SlickCellExternalCopyManager { | |||
|
|
|||
| // if a custom getter is not defined, we call serializeValue of the editor to serialize | |||
| if (columnDef) { | |||
| if (columnDef.editor) { | |||
| if (columnDef.editor?.model) { | |||
There was a problem hiding this comment.
actually I think it would be best to call the one that the SlickGrid is aware of which is editorClass since SlickGrid doesn't have any knowledge whatsoever of editor.model (you won't find editor.model anywhere in SlickGrid core file, it is assigned later by the wrapper which can be VanillaGridBundle, AngularSlickgrid, AureliaSlickgrid, ...). Note that there a few of them to replace
There was a problem hiding this comment.
thats interesting bc the examples all assign the model directly in the column definitions.
EDIT: ah ok misread, you're not using it in the core itself
hmm that also means though, while internalEditor is only deprecated but not gone we should take that first before editorClass?
There was a problem hiding this comment.
hmm that also means though, while internalEditor is only deprecated but not gone we should take that first before editorClass?
hmm I don't so, I would think that using editorClass should work, internalEditor is only used by the wrappers. I understand that it might be confusing, it might be just in my head but because I want to keep my slickgrid.core.ts file as close as possible to the original SlickGrid project. Some properties are not meant to be used within SlickGrid core file, for example any props related to export services and any plugin props and so on.. when I wasn't sure if I was going to still use 6pac/slickgrid in here, I added Generics on SlickGrid that could be extended and so when I was using it in the wrapper, I could simply provide my custom GridOption and Column interfaces from the wrapper and SlickGrid was happy with it but it's just an extra layer that I would rather not really use too much (in fact I still have a lot of places where I have grid: SlickGrid instead of the full typed ref of grid: SlickGrid<GridOption, Column> because I find it too long and I don't have to do that anymore since I merged SlickGrid in here and typically the props I want to use are already in the GridOption/Column base interfaces)... so long story short, it's kind of "only me" knowing this little thing but I'm not sure I really want to go with the full Generics+extend interface way either because I think it would overcomplicate things. I don't want to extend GridOption 3 times (1x for SlickGrid, 1x for Slickgrid-Universal, 1x. for wrapper Angular-Slickgrid... I'm already doing an extend on the wrapper and because of that I always need to release Slickgrid-Universal + all wrappers on the same day because if I don't, TypeScript complains for some reasons that I've never figured out.
slickgrid-universal/packages/common/src/core/slickGrid.ts
Lines 123 to 124 in abe772b
There was a problem hiding this comment.
hmm that also means though, while internalEditor is only deprecated but not gone we should take that first before editorClass?
the internalEditor will be removed completely in the next major and its goal was simply to hide the complexity of Editor creation to the user. I didn't want to introduce a breaking change so it's still in place and still work but will be removed completely in next major. SlickGrid always had a editor grid option (it never had the filter option though) and it was used like this editor: Slick.longText (which is simple uninstantiated Class), but since I use a few external libraries (ms-select, autocomplete, flatpickr, ...), I needed a way to potentially provide extra options and I strongly felt that the developer would still prefer to use editor without necessarily caring about how it was used internally which is why at the end I came up with that internalEditor thing, it's something that was meant to be used only within the wrapper and hidden to the user...but in some cases the user want to have access to the editor instance and modify some props (e.g. add some items to the ms-select collection) and in that case the user needed to reference the instance (not the class itself that SlickGrid editor had) and that was confusing that the developer was creating it with editor but later had to reference it with internalEditor... so this is the long story of how it all started, hope it helps understanding why I'm trying to make it simpler ;)
There was a problem hiding this comment.
ok got it. yeah I make use of the internalEditor a couple of times in my ForeignKey Filter/Editor so I understand what you're trying to simplify here.
yeah so we don't have much Cypress tests for copy+paste I guess and that is for sure not something I would think to test myself which is why it wasn't caught. There was this recent SlickGrid PR, which we should probably replicate, that added some Cypress tests for copy+paste in SlickGrid lib. We should probably do the same in here |
|
cool realPress was the command I was missing. yep will add some Cypress tests now that I know how to replicate it 💯 not today as its getting quite late over here but will get things sorted this week |
|
alright I got my change now verified with Cypress as well. I'll add others as I touch the code base again. Pretty sure there will be more to come as I continue with my projects POC to Prod conversion |
| @@ -8,6 +8,8 @@ import { | |||
| import { SlickCustomTooltip } from '@slickgrid-universal/custom-tooltip-plugin'; | |||
| import { Slicker, type SlickVanillaGridBundle } from '@slickgrid-universal/vanilla-bundle'; | |||
| import { ExampleGridOptions } from './example-grid-options'; | |||
| import { BindingEventService } from '@slickgrid-universal/binding'; | |||
There was a problem hiding this comment.
I like separating external vs internal imports, so would you mind moving this import further to the top? Thanks :)
There was a problem hiding this comment.
how about a linter rule for that? ;)
There was a problem hiding this comment.
oh does that actually exists? Dang ESLint is so powerful :P
There was a problem hiding this comment.
I'll create an additional PR after this one as it currently would involve 319 problems, pretty much all auto-fixable though ;)
There was a problem hiding this comment.
its not only separation but also unifies blank lines between those and ordered imports. So with the extra PR you get a chance to tweak those configs as you prefer.
I'll set it up tomorrow as I need to get some rest today
package.json
Outdated
| @@ -97,5 +97,8 @@ | |||
| "funding": { | |||
| "type": "ko_fi", | |||
| "url": "https://ko-fi.com/ghiscoding" | |||
| }, | |||
| "dependencies": { | |||
| "cypress-real-events": "^1.12.0" | |||
There was a problem hiding this comment.
I'm assuming this should go in Dev Dependencies instead
|
I'm not sure what's happening with Example 12 but it keeps failing with this PR, does it have any reference to |
|
meh ... example12 was the first target I wanted to look into using for E2E tests until I realized there was no copy&paste configured. I was fixing the validator condition as it didn't sound right according to the description and because I had errors when trying an inline edit. So I guess the tests build upon that bad assumption. I'll revert that again. Edit: yep thats it. But do you agree that it should be an OR instead of AND check there? |
I'm not exactly sure what you mean, would you mind explaining more? I remember adding a validation of the "Task [number]" with a RegEx, is that at all related? |
|
@ghiscoding all good, I'll create a new PR to clearly depict what I meant. As mentioned meanwhile I rolled back the change I've done here to example12 which makes the tests now pass. So this PR should be done from my side |
| container: tmpDiv, // a dummy container | ||
| column: columnDef, | ||
| position: { top: 0, left: 0 }, // a dummy position required by some editors | ||
| grid: this._grid | ||
| }); | ||
| }) as Editor; |
There was a problem hiding this comment.
@zewa666 ahh sorry, I missed this earlier, I think we can remove the any on the column and make the editorClass parsed as EditorConstructor that was added not long ago. I'm guessing we might have to explicitly parse it though
const editor = new (columnDef.editorClass as EditorConstructor)({ })`unless this other suggestion works, which would be better
const editor = new columnDef.editorClass({ }) as EditorConstructor`Note that there's 2 of these to change
There was a problem hiding this comment.
not sure if thats gonna work as editorClass can be both the ctor and an editor according to the type
There was a problem hiding this comment.
yeah ok I'll take a look at that later, I wanted to check on Stackblitz but it looks like it's blocked by the VPN on work laptop, so anyway I can review that on a separate PR if it could be improved (or not)
- this is extension to another PR #1462 that now uses `editorClass` and should be typed to `EditorConstructor` whenever possible (instead of `any` to get rid of the typing issue)
|
nope I thought you mentioned to only replicate the copy&paste tests |
|
ahh my bad, I meant that code change as well, but still I'm wondering if it's useful or not? You are definitely using it more than I do so... should we replicate it? What do you think? |
* chore(common): add `EditorConstructor` type whenever possible - this is extension to another PR #1462 that now uses `editorClass` and should be typed to `EditorConstructor` whenever possible (instead of `any` to get rid of the typing issue)
|
need to check out that one today, especially with multi cell pastes |
@zewa666 Have you had a chance to check this one? There's no rush per say, I would just like to know because I'm planning to push a new version this weekend. It could be postpone if you don't have time to check, no worries. Cheers I have 1 more possible enhancement to look into and once I push that new release then it might be the last one before the next major. I'm thinking to create a |
|
nope not yet but will do so tomorrow. I've also seen the new date lib, which could be a real improvement. especially bc flatpickr lacks some really standard stuff like a clear button in the textfield (we had to hack one by ourselves). with regards to the next branch, I think thats a good idea to have one early preview available (could also be npm published via next flags). the couple smaller PRs now where the ones my teammates and me found during our first sprint. next one starts in two weeks but I feel like we've already covered a huge area. So yeah I'll check out and if it makes sense try to squeeze in the other PRs feature so that its rdy for your next release. |
|
Hmmm ... I'm not 100% sure this is exactly a safe thing to do as focus stealing in general is regarded a bad practice. Imagine your validation lets another text box pop up where you need to write in a confirmation text. Now this PR would steal the focus and destroy your workflow. While the example is made up, generally we dont know what the user might do. Now in this exact scenario I think I would be pro this feature but I'd introduce it during the breaking change release as focus redirects can lead to undesired side-effects. So should we open an issue with a link to said PR for the sake of not forgetting it once we're around the breaking change timing? |
|
if you think it's more of a problem than a solution then we can just forget about the change. However if you think it's still worth it, then we can add it to the list of future changes or open an issue, up to you |
this is both a new feature and a bug fix.
Now when pasting if the target cell has an editor with a validator, and that one returns invalid, the onValidationError event will be triggered.
But far more sever is that the paste on target cells with editors was currently broken.
See when going to https://ghiscoding.github.io/slickgrid-universal/#/example22, setting any row into edit mode, and pasting a number from Excel lets say into the column Completion. (check the devtools).
This regression somewhat was related to this PR #1429