fix(filter): Date Filter should trigger Grid State change with Backspace

[ghiscoding]

• fixes New DateFilter's clear does not remove the components value or call stateChange #1544

TODOs

• unit tests
• add Cypress E2E test to cover this fix

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (21d8d4c) to head (097e91f).

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #1545     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         198     198             
  Lines       21671   21674      +3     
  Branches     7241    6967    -274     
========================================
+ Hits        21610   21613      +3     
  Misses         61      61             

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

[zewa666]

awesome, i can give this a try on monday with my app

[ghiscoding]

I added Cypress tests for the GraphQL Example 10 which has a Date Range filter, to cover the change, I added console log of the Grid State and it now triggers expected State change. I didn't had Cypress tests for a regular Compound Date but since the Date Range works, it should be enough (and I did tests manually the regular date filter). I could maybe add some Cypress tests for the Angular-Slickgrid (Client Side - Example 4) instead since I don't have too many examples in here with console logs of the Grid State changes... so let's merge, another bug bites the dust

Queen - Another One Bites The Dust

[zewa666]

hmm not sure but this doesn't seem to work for me on Example10 at least

[ghiscoding]

@zewa666 hmm I'll have to take another look at it later. Have you done anything troubleshooting on the code I changed? Like adding breakpoints or console logs?

EDIT

gave it another try and I see it working as expected, did you run pnpm install and are you sure that you get the latest code? Are you maybe on a Mac or does it have different keycode for whatever reason? It should fall in this event listener, you could add a breakpoint to verify. Maybe run a full Bundle Library before running it again? I don't think you have the latest code because even your date picker doesn't get cleared but mine does (see below), so you don't have latest code OR you're not falling in the listener below for unknown reason.

slickgrid-universal/packages/common/src/filters/dateFilter.ts

Lines 143 to 148 in ff61101

	// clear date picker + compound operator when Backspace is pressed
	this._bindEventService.bind(this._dateInputElm, 'keydown', ((e: KeyboardEvent) => {
	if (e.key === 'Backspace') {
	this.clear(true, false); // clear value but trigger a value change event
	}
	}) as EventListener);

[zewa666]

I was looking at it from the website samples (https://ghiscoding.github.io/slickgrid-universal/#/example10)

ooh ... hold on I guess it's not deployed there yet. that could be the reason :)

But anyways there seems to be more issues with the DatePicker, like I can now only paste formats in ISO (2020-01-07) format but not others like (07/01/2020) or (01.07.2020). So I need to dig deeper on that

[ghiscoding]

@zewa666 hahaha yeah the demo is only being deployed on releases. I did that to avoid showing unreleased features 😉

[ghiscoding]

But anyways there seems to be more issues with the DatePicker, like I can now only paste formats in ISO (2020-01-07) format but not others like (07/01/2020) or (01.07.2020). So I need to dig deeper on that

So 1 thing to note is that the new lib can only work with Date in ISO format (that is how it managed to keep its build size small) and also the associated input and its format is all done by code that I needed to implement myself (since again the external lib only accepts ISO format) and that is where and when I use the other external new lib Tempo. One advantage of having to deal with the input myself is that I no longer need parsing functions from Moment to Flatpickr and vice versa, because Flatpickr had a different parsing format than MomentJS, so I always had to do the parsing back and forth, though I no longer need to do that anymore since I'm in charge of the input format myself.

I haven't tried copy+paste so that would explain why it doesn't work, but I thought that I've set the input as read only, so how did you paste a date? Should we really accept a paste though? I mean it's a date picker, so technically you should always use the picker UI. Is that for Date editor and not the filter? ohhh unless you meant the Excel Copy Buffer plugin, yeah it might need tweaking on the parsing of the date editor (I don't remember if it's in the serializeValue(), or the loadValue, function and if that's the case then I see no date format parser in there the serializer, so it might need fixes and more unit tests). I keep forgetting to manually test the Excel Copy Buffer, we might need more tests around that feature (possibly Cypress tests as well)

[zewa666]

exactly the paste from Excel use case. My idea is anyways to allow only 3 specific formats. if its none of that I want to paste an Invalid Date forcing the user to fix it.

so ideally I could have an entry point where I do the conversion from X to ISO or error/null, so the lib can keep focusing on ISO, which I very like btw. Where would you see that paste check fit?

[ghiscoding]

I'm not sure how the Excel Copy Buffer works, I added console log on the date editor serializeValue() and loadValue() but it doesn't go in there when pasting. It probably makes sense since at that point we don't have actually have the editor opened, so it must be coming from somewhere else but in that case it might just be a straight copy+paste without validation of any kind? You probably more than I do on how the plugin works since you worked on extending Example 19 with that kind of validation!?

Was the same manual test actually working before the new major?

[zewa666]

yes pasting has a somewhat different workflow as it only consumes the validator from a defined editor (one of the earlier PRs).

I didnt have that feature yet and sat down to implement it since the old parser actually acted a but weird from different locales based excel sheets. hence I'm actually quite happy the new one just accepts ISO and forces it on the dev to convert the copy buffer to the matching format.

I guess I'll go with onBeforePasteCell and simply do the conversion there if the target column to be pasted from the range is a date editor. So no worries I'll figure it out. perhaps it makes sense to update the docs afterwards with a snippet on how others can do it as well

[ghiscoding]

yeah ok, I'll let you submit an update of the docs if you don't mind since you know the subject more than I do. Just 1 thing to note though is that when parsing with Tempo, I purposely used en-US in my code because since I'm in Canada, I had a formatting conversion problem when running locally (I had en-CA) vs CI (en-US) and the format was slightly different making my PR fails in CI, so to avoid format differences you can set a static locale to use in Tempo. The problem I had in my case is that on my local it was showing a.m. but in the CI it was showing as am (I even opened an issue in Tempo and got clarification from the maintainer)... so in summary, it's just some "gotcha" to be informed of. Finally the thing to note is that Tempo tries to use Intl.DateTimeFormat as much as possible to keep the lib small, which is why I had some format differences

slickgrid-universal/packages/common/src/filters/dateFilter.ts

Lines 304 to 305 in aa0a74f

	const firstDisplayDate = format(self.selectedDates[0], outputFormat, 'en-US');
	const lastDisplayDate = format(lastDate, outputFormat, 'en-US');

[zewa666]

yeah being from Austria I'm used to all sorts of weird stuff with i18n related conversion issues between en-US and de-AT. seen that one you mentioned and I think I've also proposed fixing everything CI related to en-US, at least thats what I used to do with Aurelia-I18n

[ghiscoding]

I just edited my comment above with the following comments which explains it all I guess...

Finally the thing to note is that Tempo tries to use Intl.DateTimeFormat as much as possible to keep the lib small, which is why I had some format differences

[zewa666]

btw whats your recommended approach to testing out a non-released master merge with my angular-slickgrid app? just forcefully npm install straight from github-master vs npm? or should I do a local build of master repo and npm link?

[ghiscoding]

my approach is very uncommon and crude, I run a full Slickgrid-Universal build and then copy the dist into each Angular-Slickgrid node_modules/@slickgrid-universal/ packages that changed, it's crude and long. Also since I prefer to stay on Windows 10 for the time being, I don't have Tabs in file explorer so I use Q-Dir (as in Quad Directory Explorer) a small Windows freeware that allows me to use multiple tab in the same window and also allows to save your view in a favorite so it's a lot easier to manage with this freeware (see this youtube video about it). My favorite is 4 tabs which is great since I have 4 repos to manage lol (Slickgrid-Universal and 3 other repos that uses it).. talking about Q-Dir, I forgot that they had a Portable App package, which is great since I can't install anything on my work laptop but the Portable App works, awesome I can use it at work too :)

Another approach that is typically recommended is to use npm link (something like along this article), it's probably better with yarn or pnpm though... but 1 of the reason I'm not using this approach is that there's no easy On/Off switch to remove this link, so once it's linked it kind of bypasses the original and released code.

if you find something better, please let me know, for now I'll keep using 1st approach

Looking at pnpm docs, I guess you could do 3rd approach that you mentioned which would probably work too "bar": "npm:foo", this should work when the code is merged in the main branch

[zewa666]

i thought going with npm pack https://dev.to/scooperdev/use-npm-pack-to-test-your-packages-locally-486e

[ghiscoding]

oh cool I wasn't aware of that one, but you probably need to find out the full path to get it working and update all packages, it shouldn't be too much of a problem. Have you tried the 3rd approach I mentioned above? Basically what you proposed earlier

[zewa666]

nope, I actually prefer to stick to npm, thats the baseline for everyone, and only use pnpm in your repos. So that wont work for my app

[ghiscoding]

if you want to go with npm pack, you could add npm scripts for each package (I've done that in Lerna-Lite for testing purposes) "pack-tarball": "npm pack"