Created attachment 166138 [details] [review] Bug 36774: [DONT PUSH] TEST ONLY Test plan: 1) Apply only the TEST patch 2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl 3) Click the "Expires before" input and insert any date 4) Click the "TEST" link under. It calls the native flatpickr.clear(): https://flatpickr.js.org/instance-methods-properties-elements/#clear 5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input. 6) Apply 2nd patch. Repeat. Notice it works as expected
Created attachment 166139 [details] [review] Bug 36774: Dont add wrapper if already exists
Context at bug 34263
There is also calendar.inc at opac-tmpl/bootstrap/en/includes/calendar.inc and this change further diverges both files. It's a dupe of this file but for the OPAC (see bug 35716 if you're interested in this topic i.e. stop duping assets for OPAC+INTRA)
Created attachment 166162 [details] [review] Bug 36774: [DONT PUSH] TEST ONLY Test plan: 1) Apply only the TEST patch 2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl 3) Click the "Expires before" input and insert any date 4) Click the "TEST" link under. It calls the native flatpickr.clear(): https://flatpickr.js.org/instance-methods-properties-elements/#clear 5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input. 6) Apply 2nd patch. Repeat. Notice it works as expected Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166163 [details] [review] Bug 36774: Dont add wrapper if already exists Signed-off-by: David Nind <david@davidnind.com>
Added assignee
Bug 30718: Prevent 'reset' icon to appear twice - $(instance.input).siblings('input.flatpickr') + $(instance.input).find('~input.flatpickr:first') Maybe we need to adjust this line as well then because it was supposed to deal with that problem.
(In reply to Jonathan Druart from comment #8) > Bug 30718: Prevent 'reset' icon to appear twice > > - $(instance.input).siblings('input.flatpickr') > + $(instance.input).find('~input.flatpickr:first') > > Maybe we need to adjust this line as well then because it was supposed to > deal with that problem. I don't know. From my testing, that change from bug 30718 didn't fix the problem it said it fixed, i.e. the problem is there before and after that change. However, reverting it back to the previous code makes the "x" button stop working, it doesn't clear anymore. I don't have more time to dig deeper here but unless we come up with a test plan highlighting why we need to adjust that line I'd say it's a non-issue imo.
I guess it was supposed to fix this: From bug 30718 comment 22 """ I notice also that paired date fields (start date & end date) now show an extra "X" icon after the second field, e.g. serial claims or catalog statistics. """
(In reply to Jonathan Druart from comment #10) > I guess it was supposed to fix this: > > From bug 30718 comment 22 > """ > I notice also that paired date fields (start date & end date) now show an > extra "X" icon after the second field, e.g. serial claims or catalog > statistics. > """ Tested both entries of the JS line below on the ILL requests page (there are 4 flatpickr instances on the left side filters): > - $(instance.input).siblings('input.flatpickr') > + $(instance.input).find('~input.flatpickr:first') I couldn't spot any difference before and after. Mind you I didn't test the clear itself because those filters 'clear' functionality needs to be updated, but the rendering of the inputs is fine, no additional "x" anywhere are rendered. Also looked at /tools/holidays.pl but can't find any issue.
On main, if I revert commit 97a4b3c61da522f35628f80382d21bbff56b76ba Bug 30718: Prevent 'reset' icon to appear twice I am seeing: https://snipboard.io/XLKMVg.jpg on /cgi-bin/koha/serials/claims.pl?supplierid=1 KO (2 clears for "to") On main, I am seeing: https://snipboard.io/u728qI.jpg OK 1 clear for "from", 1 for "to" On main + your patch: https://snipboard.io/6BusJT.jpg KO 1 clear for "from" but no clear for "to"
Created attachment 166278 [details] [review] Bug 36774: Ensure we are looking for a parent with the given class And not one of its children
I think you want that.
Comment on attachment 166278 [details] [review] Bug 36774: Ensure we are looking for a parent with the given class Hi Joubu, thank you very much for the follow-up, it fixes the claims page issue but it causes my test plan to fail.
Created attachment 166303 [details] [review] Bug 36774: [DONT PUSH] TEST ONLY Test plan: 1) Apply only the TEST patch 2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl 3) Click the "Expires before" input and insert any date 4) Click the "TEST" link under. It calls the native flatpickr.clear(): https://flatpickr.js.org/instance-methods-properties-elements/#clear 5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input. 6) Apply 2nd patch. Repeat. Notice it works as expected Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166304 [details] [review] Bug 36774: Dont add wrapper if already exists Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166305 [details] [review] Bug 36774: (QA Follow-up): Use flatpickr clear Im not sure why, but with the way it was before, there was a bug in my test plan. If you cleared using the TEST link, and then input some date and clear with the 'x', a JS error would occurr.
(In reply to Jonathan Druart from comment #12) > On main, if I revert > commit 97a4b3c61da522f35628f80382d21bbff56b76ba > Bug 30718: Prevent 'reset' icon to appear twice > > I am seeing: https://snipboard.io/XLKMVg.jpg on > /cgi-bin/koha/serials/claims.pl?supplierid=1 KO (2 clears for "to") > > > On main, I am seeing: https://snipboard.io/u728qI.jpg OK 1 clear for "from", > 1 for "to" > > On main + your patch: https://snipboard.io/6BusJT.jpg KO 1 clear for "from" > but no clear for "to" Thanks for this. I've submitted a squashed follow-up to the original patch and a follow-up to fix a bug I discovered while re-testing this, explanation in the commit message. I had a hard time following your claims use case, but in the end I was able to and I think I discovered a bug in it as well. Filed bug 36804.
Created attachment 166383 [details] [review] Bug 36774: [DONT PUSH] TEST ONLY Test plan: 1) Apply only the TEST patch 2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl 3) Click the "Expires before" input and insert any date 4) Click the "TEST" link under. It calls the native flatpickr.clear(): https://flatpickr.js.org/instance-methods-properties-elements/#clear 5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input. 6) Apply 2nd patch. Repeat. Notice it works as expected Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166384 [details] [review] Bug 36774: Dont add wrapper if already exists Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166385 [details] [review] Bug 36774: (QA Follow-up): Use flatpickr clear Im not sure why, but with the way it was before, there was a bug in my test plan. If you cleared using the TEST link, and then input some date and clear with the 'x', a JS error would occurr. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166395 [details] [review] Bug 36774: Dont add wrapper if already exists Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 166396 [details] [review] Bug 36774: (QA Follow-up): Use flatpickr clear Im not sure why, but with the way it was before, there was a bug in my test plan. If you cleared using the TEST link, and then input some date and clear with the 'x', a JS error would occurr. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
It would be nice to have a cypress test for this, but not considering blocker.
There's a flaw in my code. It's changing the css class to flatpickr_wrapper_(id of the element), thus causing CSS rules to flatpickr_wrapper to not apply. See issues caused at <staff_url>/cgi-bin/koha/circ/overdue.pl I opted for this approach intentionally in case the flatpickr input does not contain an id, but it's not great, need to revisit.
Created attachment 166423 [details] [review] Bug 36774: [DONT PUSH] TEST ONLY Test plan: 1) Apply only the TEST patch 2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl 3) Click the "Expires before" input and insert any date 4) Click the "TEST" link under. It calls the native flatpickr.clear(): https://flatpickr.js.org/instance-methods-properties-elements/#clear 5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input. 6) Apply 2nd patch. Repeat. Notice it works as expected Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166424 [details] [review] Bug 36774: Remove wrapper if already exists Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 166425 [details] [review] Bug 36774: Add cypress tests
Created attachment 166427 [details] [review] Bug 36774: Add cypress tests Inside k-t-d, run: cypress run --spec t/cypress/integration/flatpickr_spec.ts
(In reply to Pedro Amorim from comment #18) > Created attachment 166305 [details] [review] [review] > Bug 36774: (QA Follow-up): Use flatpickr clear > > Im not sure why, but with the way it was before, there was a bug in my test > plan. > If you cleared using the TEST link, and then input some date and clear with > the 'x', a JS error would occurr. I now know why, flatpickr().clear() is re-adding the 2nd input and triggering onReady but because onReady was returning early it was not re-wrapping the flatpickr_wrapper, causing this error. In hindsight, I don't think we should be adding this clear button on onReady but use wrap from flatpickr instead: https://flatpickr.js.org/examples/#flatpickr-external-elements But I've invested enough time here and this is too much work for little gain. Joubu, David, apologies for the back and forth here, this turned out to be a much deeper rabbit hole than I thought. I've added cypress tests (more tests on the claims page and patron hold tab could be added though) and went through the test plans in bug 34263 and bug 36804 again to make sure everything is working as expected, I believe it is.
Created attachment 166463 [details] [review] Bug 36774: Remove wrapper if already exists Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166464 [details] [review] Bug 36774: Add cypress tests Inside k-t-d, run: cypress run --spec t/cypress/integration/flatpickr_spec.ts Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.06
Backported to 23.05.x for upcoming 23.05.12