Bug 34707 - Abstract intranet-tmpl/includes/*-search.inc
Summary: Abstract intranet-tmpl/includes/*-search.inc
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-04 16:01 UTC by Pedro Amorim
Modified: 2023-09-06 07:11 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 34707: New common/header-search-tabs.inc (2.94 KB, patch)
2023-09-04 16:04 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34707: Apply *-search.inc abstraction to cities (3.15 KB, patch)
2023-09-04 16:04 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-09-04 16:01:42 UTC
After looking at bug 34624, I thought we could abstract (thus reducing) a lot of code and, making it more maintainable.
Comment 1 Pedro Amorim 2023-09-04 16:04:27 UTC
Created attachment 155202 [details] [review]
Bug 34707: New common/header-search-tabs.inc

Proof of concept of new abstract header-search-tabs.inc to be called everywhere its needed
Comment 2 Pedro Amorim 2023-09-04 16:04:30 UTC
Created attachment 155203 [details] [review]
Bug 34707: Apply *-search.inc abstraction to cities

PROOF OF CONCEPT
I've applied this idea to cities to start discussion around this.
Comment 3 Pedro Amorim 2023-09-04 16:05:13 UTC
I believe we can abstract this even further but, for now, test plan is to apply patches and play around cities. Check that the top search bar works as before, etc.
Comment 4 Jonathan Druart 2023-09-06 07:10:50 UTC
I am not sure, that's a lot of different variables to define. Also some inc files are included from different places, you are going to duplicate this new big block.

Another approach (to test!) would be to define all the 'tabs' in a single include, then pass the idea from the tt?

include would contain all those blocks (here is one):
+    {
+        id          => 'city_search',
+        is_active   => 1,
+        is_allowed  => 1,
+        label       => 'Search cities',    #TODO:translate
+        icon        => 'fa-building',
+        form_action => undef,
+        form_id     => undef,
+        form_method => 'POST',
+        inputs      => [
+            {
+                id          => 'city_name_filter',
+                name        => 'city_name_filter',
+                type        => 'text',
+                value       => city_name_filter,
+                placeholder => 'City search',
+                class       => 'head-searchbox form-control',
+            }
+        ]
+    },

and tt would simply include:
[% INCLUDE header-search-tabs.inc tabs => ['city-search', 'circ_search', 'catalog_search'] %]
Comment 5 Jonathan Druart 2023-09-06 07:11:27 UTC
(In reply to Jonathan Druart from comment #4)
> I am not sure, that's a lot of different variables to define. Also some inc
> files are included from different places, you are going to duplicate this
> new big block.
> 
> Another approach (to test!) would be to define all the 'tabs' in a single
> include, then pass the idea from the tt?
> 
> include would contain all those blocks (here is one):
> +    {
> +        id          => 'city_search',
> +        is_active   => 1,
> +        is_allowed  => 1,
> +        label       => 'Search cities',    #TODO:translate
> +        icon        => 'fa-building',
> +        form_action => undef,
> +        form_id     => undef,
> +        form_method => 'POST',
> +        inputs      => [
> +            {
> +                id          => 'city_name_filter',
> +                name        => 'city_name_filter',
> +                type        => 'text',
> +                value       => city_name_filter,
> +                placeholder => 'City search',
> +                class       => 'head-searchbox form-control',
> +            }
> +        ]
> +    },
> 
> and tt would simply include:
> [% INCLUDE header-search-tabs.inc tabs => ['city-search', 'circ_search',
> 'catalog_search'] %]

s/idea/ids