Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve core "featured records" functions #1003

Open
zerocrates opened this issue Jan 27, 2023 · 4 comments
Open

Improve core "featured records" functions #1003

zerocrates opened this issue Jan 27, 2023 · 4 comments
Assignees

Comments

@zerocrates
Copy link
Member

Themes are almost all replacing these with their own and it's caused duplicated work.

The features these provide should be something we can have in the core to ease the burden on themes. Minimally this is something like having options for multiple where the core/plugin function is currently single, and possibly more. A survey of what the themes are doing is probably necessary to see what we should be providing.

@kimisgold kimisgold self-assigned this Feb 13, 2025
kimisgold added a commit that referenced this issue Feb 18, 2025
This helper consolidates other featured record displays.

* It takes multiple types of records.
* Allows users to pass overrides for the partial that renders the
  featured records.
* Allows users to set thumbnail size for representative images.
@kimisgold
Copy link
Member

I documented the custom featured record functions implemented across the themes:

  • Thanks, Roy: The core only has random_featured_collection() and exhibit_builder_random_featured_exhibit(), which display a single record of their respective types. Thanks, Roy’s custom function allows for multiple collections and exhibits to be displayed.
  • Big Picture, Center Row: Displays featured records in a slider, so allows for both multiple collections and exhibits to display, but also in a partial with custom markup to be applied to all records.
  • Foundation: Provides 3 areas for displaying a featured record. The theme configuration allows the site admin to designate what record type goes in each section. These settings are passed to the custom random featured function, which then find the respective single partial to render.
  • Freedom: Same as Foundation, also enables a featured timeline.
  • The Daily: Displays all available featured records for items, collections, and exhibits. Uses default single.php for rendering.

So to summarize, the uses cases typically want to be able to pass a record type, number of records to retrieve, and define a partial to use. In addition, as the current random_featured_items() function also allows to select records with images, it might be useful for the user to be able to define a preferred thumbnail size. Currently random_featured_items() defaults to "fullsize".

I've drafted random_featured_records() on the random-featured-records branch. There are branches with the same name in Big Picture, Foundation, and Center Row.

@zerocrates
Copy link
Member Author

zerocrates commented Feb 21, 2025

Summarizing some thoughts on random_featured_records:

  • $overrides is probably more clearly just called $partials (and also see below for discussion on single vs. multiple call)
  • $hasImage isn't getting passed through to the query, but this leads to a bigger question... would we prefer this to be a more generic function and let theme writers just pass the query directly, so they could change it to a random selection of all items, a non-random selection, to require images or not, or do some totally different query? I think that's probably a good idea... though we'd probably want to name the function something slightly different...
  • As currently written this is skipping one of the most important areas where there were issues between the themes' implementations: handling presence/absence of Exhibit Builder. Probably the best way forward on this is to convert the static list of partials to something that just includes the core records/partials, and is passed to a filter, where plugins can modify the list to add support for their records. Trying to print the featured records for a record type not in the list would just be a no-op and return nothing, getting us handling of plugins being active/inactive/uninstalled "for free" and also allowing extension to other plugins (Geolocation? Timeline?)
  • Similarly the call for the exhibit builder filter event probably shouldn't live here, instead there should be some new filter for this function's output... probably just one filter that passes along all the parameters, to let a filtering plugin decide if it wants to take action by looking at the type, or whatever else
  • The other major question I have is: should this be rewritten to just return record display for one record type at a time, requiring multiple calls if a theme wants to show multiple record types together? It's simpler to write and use the function if it's single: parameters that here need to be type-indexed arrays or nested to an extra level could be simple scalars or one-dimensional arrays. And the themes are pretty sharply divided in how they call this kind of function, with many using separate calls already, to display the records in separate areas, with separate headings, etc.

@kimisgold
Copy link
Member

would we prefer this to be a more generic function and let theme writers just pass the query directly, so they could change it to a random selection of all items, a non-random selection, to require images or not, or do some totally different query? I think that's probably a good idea... though we'd probably want to name the function something slightly different...

I'm open to this. I can definitely see more uses for this outside of a "featured" context. Maybe something as simple as display_records()?

The other major question I have is: should this be rewritten to just return record display for one record type at a time, requiring multiple calls if a theme wants to show multiple record types together? It's simpler to write and use the function if it's single: parameters that here need to be type-indexed arrays or nested to an extra level could be simple scalars or one-dimensional arrays. And the themes are pretty sharply divided in how they call this kind of function, with many using separate calls already, to display the records in separate areas, with separate headings, etc.

Yeah, I think I'm leaning towards something more straightforward like that. It would probably make considerations for displaying plugin record types easier as well.

@kimisgold
Copy link
Member

2413163 responds to the latest feedback. It's been renamed display_records(), and takes a record type, query parameters for get_records(), a partial path, an array of params to pass to the partial, and a limit of records to retrieve. Center Row, Big Picture, and Foundation have updated implementations to look at as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants