Description
Jonathan Druart
2012-05-18 09:57:12 UTC
Created attachment 9634 [details] [review] Bug 5849: Divide budget periods into two tabs Created attachment 9635 [details] [review] Bug 8117: Divide budget periods into two tabs Created attachment 10138 [details]
screenshot of budget page
Hi Jonathan,
the javasript page on the page seems to be broken for me. I tried refreshing the cache and closing the browser completely. I think it should not be separate tables but tabs?
Perhaps patch could also make use of the Koha dates plugin? :)
Created attachment 10160 [details] [review] Bug 8117: Followup: FIX compatibility for the new version of jQuery tabs Created attachment 10161 [details] [review] Bug 8117: Followup: Use the KohaDates plugin in aqbudgetperiods.tt Created attachment 10178 [details] [review] [SIGNED-OFF] Bug 8117: Divide budget periods into two tabs Budget periods are now divided into two tabs : active and inactive budget periods. The original author of this patch is Julian Maurice Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 10179 [details] [review] [SIGNED-OFF] Bug 8117: Followup: FIX compatibility for the new version of jQuery tabs Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 10180 [details] [review] [SIGNED-OFF] Bug 8117: Followup: Use the KohaDates plugin in aqbudgetperiods.tt Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Looks really good, and I think this is a really good feature. However, it fails a javascript validation error when the patch is applied. To recreate, add two active budgets, apply patch, refresh budgets page. DataTables warning (table id = 'inactiveperiodst'): Requested unknown parameter '1' from the data source for row 0 Created attachment 10231 [details] [review] Bug 8117: Followup Move the "no budget" string outside the table Hi Michael, Thanks for testing. I can't reproduce your javascript error. Could you test with this followup please ? Created attachment 10235 [details] [review] [SIGNED-OFF] Bug 8117: Followup Move the "no budget" string outside the table Signed-off-by: Michael Davis <m.r.davis@cranfield.ac.uk> This works for me, thanks, but I may have messed up the patches, so someone else should sign off. Applying the patches fails for me at this step: Applying: Bug 8117: Followup: Use the KohaDates plugin in aqbudgetperiods.tt fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 8117: Followup: Use the KohaDates plugin in aqbudgetperiods.tt Created attachment 11047 [details] [review] Bug 8117: Divide budget periods into two tabs Squashed and rebased path A quick last signoff is required. Created attachment 11052 [details] [review] [SIGNED-OFF] Bug 8117: Divide budget periods into two tabs Budget periods are now divided into two tabs : active and inactive budget periods. The original author of this patch is Julian Maurice Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Michael Davis <m.r.davis@cranfield.ac.uk> Signed-off-by: Owen Leonard <oleonard@myacpl.org> QA comment: * small patch, just for a better display * passes test-qa.pl * A fix and a question: - you weren't respecting [HTML4] coding guideline (Active Budget should have been Active budget). I've fixed them - why do you repeat (as <h3> the title of the tab at the beginning ot the tab ?) * is it me or the toolbar does not display nice buttons but basic links New budget, New, Budgets, Budget planning are in blue, not styled. I tried shift-reload, switching to another page, switching to another language, the problem is still here pls check my last point (In reply to comment #16) > QA comment: > - you weren't respecting [HTML4] coding guideline (Active Budget should > have been Active budget). I've fixed them Thanks! > - why do you repeat (as <h3> the title of the tab at the beginning ot > the tab ?) You're right. It's useless, I will submit a followup > * is it me or the toolbar does not display nice buttons but basic links New > budget, New, Budgets, Budget planning are in blue, not styled. I tried > shift-reload, switching to another page, switching to another language, the > problem is still here I don't reproduce this behaviour. Buttons display correctly for me. Created attachment 11325 [details] [review] Bug 8117: Followup: remove h3 title (duplicate infos) *** Bug 8654 has been marked as a duplicate of this bug. *** QA comment: this time it's OK ! passed QA A follow-up will be added though, because we understood why I had some javascript error and jonathan didn't. It's because I've plack + debug debug panel in plack adds a lot of <table> (without any id) thus, when you call $("table").dataTable(...) there's a javacscript error because datatable can't handle the plack <table> ! The solution has been to explicitely name the tables to dataTable (which is a good thing anyway) $("#activeperiodst,#inactiveperiodst").dataTable($.extend(true, {}, dataTablesDefaults, { Patch pushed to master Created attachment 13713 [details] [review] Bug 8117 [Follow-up] Divide budget periods into two tabs This patch corrects some capitalization errors. Created attachment 13727 [details] [review] Bug 8117 [Follow-up] Divide budget periods into two tabs This patch corrects some capitalization errors. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 13821 [details] [review] [SIGNED-OFF] Bug 8117 [Follow-up] Divide budget periods into two tabs This patch corrects some capitalization errors. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> String changes, all tests pass. This patch has been pushed to master. Pushed to 3.10.x, will be in 3.10.1 |