Feature/content search #172

wykhuh · 2024-02-23T00:52:39Z

@adamjarling @mathewjordan Add content search for content search 2.0. Working demo of content search
http://localhost:3000/docs/viewer/newspaper

Screen.Recording.2024-02-22.at.4.39.17.PM.mp4

the PR is based on Clover 2.4.0-rc.0
use iiifContentSearch prop on Viewer to create a search for 'Berliner' when Clover loads.
To make the search demo work, I had to add an API endpoint in Next.js for the search service.
http://localhost:3000/api/newspaper_search/1?q=
We need to remove the the two commits for the demo search service before the PR is merged into main (add api endpoint to create search manifests for newspaper, update newspaper manifest to work with dynamic content search service)
Instead of hardcoding text in the components, I added configOptions.localeText.contentSearch to store text that is used in content search UI. Not sure if this is the best place to store text strings that appear in the UI.
add configOptions.contentSearch.searchResultsLimit to limit the number of search results to show in the information panel. All the search results will be highlighted, but if searchResultsLimit is set, only xxx number of results will be shown per canvas.
the search results are grouped by canvas in the information panel.
We want search results to be the first tab in the informational panel. We realize other projects might want another tab to be first. Any suggestions on how users can set the order of the tabs?

mathewjordan · 2024-02-27T21:09:25Z

Hey @wykhuh. This is excellent work and will be a boon to Clover and IIIF in general. I'm reviewing it in detail now and will add some inline code suggestions and comments, but a few quick high level notes:

Static Search Endpoints - I love the addition of the mock search api. I'm wondering if we could simply mock this as static files in public/api/search/..., that way we can set the export param to true in the next.config.js AND include the mock search fixture in Clover's code.
Tab Order - What do you think about ordering of tabs, perhaps somehow setting default tab if Search API is being utilized? I think keeping the About tab as the first in the DOM would be preferred, even if Search Results is somehow opened as the default tab if iiifContentSearch is present, ex: About | Search Results | Annotations
Can we make the Newspaper fixture and corresponding parts more generic and name it in relation to Content Search? If we did this, I think we can keep it as part of docs.

mathewjordan · 2024-02-27T23:06:46Z

@wykhuh This is probably it for now. It looks great. Functions great. I'm impressed by the search results across canvases as well. I think that is something that is really useful. Can you take a look at my comment at let me know if they make sense? I'd be happy to review any new commits as they come in.

wykhuh · 2024-02-28T17:52:30Z

@wykhuh This is probably it for now. It looks great. Functions great. I'm impressed by the search results across canvases as well. I think that is something that is really useful. Can you take a look at my comment at let me know if they make sense? I'd be happy to review any new commits as they come in.

@mathewjordan Getting the search result to work across canvases took a little bit of experimenting. :-)

(1 & 3) I think having a demo search in the Clover docs would be good. However, I haven't tried building the Clover website, so I don't know what is the best way to setup a mock search api and use more generic names for the docs. Do you have time to make the changes? You have access to my repo, so you could add more commits to this branch.

(2) We would prefer to reorder the tabs rather than setting the search tab to open by default. It's part of the overall conversation of how to make it easier for people to adjust the UI Clover to fit their project needs.

collectiveaccess · 2024-02-28T18:04:38Z

On the kind-of-related note I saw another developer offer to localize Clover. Localization would go a long way towards enabling customization, as a custom locale could be used to relabel things.

wykhuh · 2024-04-18T18:27:19Z

@adamjarling @mathewjordan I've updated this content search to Clover 2.7.5

mathewjordan

Closing out previous review...

mathewjordan · 2024-02-27T22:42:40Z

pages/docs/viewer/_meta.json

@@ -5,5 +5,12 @@
      "sidebar": false
    },
    "title": "Demo"
+  },
+  "newspaper": {
+    "title": "Newspaper",


I actually like having a content search demo page. To retain it, can we rename Newspaper to Content Search, where applicable?

We can keep this if it defaults to a manifest fixture live on the internet or in the IIIF Cookbook.

IIIF cookbook does not have a content search manifests. I asked our client the Grand Rapids Public Library if it is okay to use one of their newspaper manifest with content search on the Clover site. I'll let you know they say.

Grand Rapids Public Library doesn't have a publicly available manifest with search service. I ended up using a static content search manifest for the content search demo. When people type in a search term, Clover will show the search results for "Berliner" using the newspaper issue from IIIF cookbook. Do you think this will work for the content search page or is it kinda confusing? The content search demo is at /docs/viewer/contentsearch

If we can find a working example of a manifest with a search service, we can update the iiifContent on the content search page to have fully functional search demo.

mathewjordan · 2024-02-27T22:43:52Z

public/manifest/content-search/content-search.json

@@ -0,0 +1,49 @@
+{
+  "@context": "http://iiif.io/api/search/2/context.json",
+  "id": "http://localhost:3000/manifest/newspaper/content-search.json",


And again, maybe just makes this content search and not newspaper.

mathewjordan · 2024-02-27T22:50:45Z

src/context/viewer-context.tsx

+  localeText: {
+    contentSearch: {
+      tabLabel: "Search Results",
+      formPlaceholder: "Enter search words",
+      noSearchResults: "No search results",
+      loading: "Loading...",
+      moreResults: "more results",
+    },
+  },


Thank you for doing this. I recognize that it might be difficult to implement locale language content without better structures in place. I may move this in a near future to a locale specific location similar to how Mirador handles it, ex: https://github.com/ProjectMirador/mirador/tree/master/src/locales but for now this should be fine. Thanks for doing this.

I saw that y'all are working on i18. I left the localeText in the code, but didn't update the docs. If content search gets merged into main branch before your i18 PR, then y'all can update the localeText contentSearch code.

mathewjordan · 2024-02-27T22:58:01Z

src/components/Viewer/InformationPanel/InformationPanel.tsx

+    if (activeResource) {
+      return;
+    } else if (renderContentSearch) {
+      setActiveResource("manifest-content-search");
+    } else if (renderAbout) {


❗ Something here appears to be impacting About being active by default when the renderContentSearch is not true.

See http://localhost:3000/docs/viewer/demo where the About tab needs to be manually activated to show content.

I fixed this issue.

mathewjordan · 2024-02-27T23:03:48Z

src/components/Viewer/InformationPanel/ContentSearch/ContentSearchForm.tsx

+    <FormStyled>
+      <Form.Root onSubmit={searchSubmitHandler} className="content-search-form">
+        <Form.Field name="searchTerms" onChange={handleChange}>
+          <Form.Control placeholder={searchText?.formPlaceholder} />


❗ This doesn't appear to render accessibly (contrast wise) in dark mode. Seems like it just needs some contrast on the color. We use the $primary color token for this currently.

Disregard the use of $primary here. It may be better to inherit the color.

I removed the backgroundcolor definition from css.

mathewjordan

I think we're almost there. I didn't get too in the weeds on things but what if update to match the shift to iiifConteSearchQuery in the props?

In addition to the below suggestions:

Differentiate Annotation Overlays somehow?

We may need to differentiate the Search Result and Annotation overlays rendered in OpenSeadragon.

In this example, the annotation overlay for the masthead iconography is showing alongside search results:

mathewjordan · 2024-04-26T15:41:16Z

pages/docs/viewer/newspaper.mdx

+
+<Viewer
+  iiifContent="http://localhost:3000/manifest/newspaper/newspaper_issue_1.json"
+  iiifContentSearch="http://localhost:3000/api/newspaper_search/1?q=Berliner"


To be closer to spec and make things relative to the service on the Manifest, what if we rename the prop to iiifContentSearchQuery and allow the shape of that to match https://iiif.io/api/search/2.0/#query-parameters.

Suggested change

iiifContentSearch="http://localhost:3000/api/newspaper_search/1?q=Berliner"

iiifContentSearchQuery={{

q: "Berliner",

}}

In this model, the Viewer is not handed the endpoint, but relies on the stated endpoint in the Manifest, ex:

"service": [ { "id": "http://localhost:3000/api/newspaper_search/1", "type": "SearchService2" } ]

In this method, the Search Results tab in the information would be rendered if the Manifest itself had a defined Content Search service, maybe just support SearchService2 for now and not being backwards compatible. If iiifContentSearchQuery does not exist, then results would be empty, but the tab would still be displayed. If q is set, then that would be submitted, showing results.

If q is set on the iiifContentSearchQuery prop, the value of q should be represented in the Search Results input element. Ex:

I updated the props to be

iiifContentSearchQuery={{ q: "Berliner", }}

mathewjordan · 2024-04-26T15:55:24Z

src/components/Viewer/InformationPanel/InformationPanel.tsx

+        {renderContentSearch && contentSearchResource && (
+          <Trigger value="manifest-content-search">
+            <Label label={contentSearchResource.label as InternationalString} />
+          </Trigger>
+        )}


Depending on the application, the About tab may be the one expected to be rendered by default. Instead of making tab order configurable, what if we reset ordering to be:

About

Search Results

Annotations

And have a prop options.informationPanel.defaultTab that mapped to the defaultValue (the poorly named activeResource) of Wrapper (Tabs.Root). We would need a check to make sure that the value exists in in the Tab List. In Collective Access application, the defaultTab could be: manifest-content-search.

I put the about tab first and added options.informationPanel.defaultTab prop.

mathewjordan · 2024-04-26T15:57:29Z

next.config.js

@@ -13,6 +13,6 @@ module.exports = (phase) => {
    images: {
      unoptimized: true,
    },
-    output: "export",
+    // output: "export",


We need to reset this before we publish for hosting of docs on github pages.

I reset next.config.js

mathewjordan · 2024-04-26T15:58:06Z

docs/components/DynamicImports/Viewer.tsx

@@ -33,6 +35,7 @@ const CloverViewer = ({
  return (
    <Viewer
      iiifContent={iiifResource}
+      iiifContentSearch={iiifContentSearch}


Rename to match iiifContentSearchQuery noted above.

Suggested change

iiifContentSearch={iiifContentSearch}

iiifContentSearchQuery={iiifContentSearchQuery}

mathewjordan · 2024-04-26T16:00:10Z

docs/components/DynamicImports/Viewer.tsx

 }: {
  iiifContent: string;
  options?: ViewerConfigOptions;
  customDisplays?: Array<CustomDisplay>;
+  iiifContentSearch?: string;


I think we can get by with just q for now but we might will want to define this type to fit the shape of https://iiif.io/api/search/2.0/#query-parameters as we extend functionality. This gives us room to grow.

Suggested change

iiifContentSearch?: string;

iiifContentSearchQuery?: {

q: string;

};

mathewjordan · 2024-04-26T16:03:34Z

pages/api/newspaper_search/[id].tsx

@@ -0,0 +1,61 @@
+import type { NextApiRequest, NextApiResponse } from "next";


What if we reorganize this api handler to be in a fixtures directory at pages/api/fixtures/newspaper_search/[id].tsx. This will make it clear that this is for development purposes.

I updated the path to pages/api/fixtures/content-search/[id].tsx. The api endpoint doesn't work right now since I reset next.config.js output: "export". If any needs to use the api during development, comment out output: "export".

pages/api/newspaper_search/[id].tsx

mathewjordan · 2024-04-26T16:13:45Z

pages/docs/viewer/_meta.json

@@ -5,5 +5,12 @@
      "sidebar": false
    },
    "title": "Demo"
+  },
+  "newspaper": {
+    "title": "Newspaper",


We can keep this if it defaults to a manifest fixture live on the internet or in the IIIF Cookbook.

wykhuh · 2024-04-30T00:14:34Z

Differentiate Annotation Overlays somehow?

We may need to differentiate the Search Result and Annotation overlays rendered in OpenSeadragon.

I added contentSearch.overlays to ViewerConfigOptions. Now there are two sets of overlays config: one for annotations and one for content search. By default, I made the content search overlays red, and the annotations overlays blue.

mathewjordan · 2024-04-30T14:50:40Z

Differentiate Annotation Overlays somehow?

We may need to differentiate the Search Result and Annotation overlays rendered in OpenSeadragon.

I added contentSearch.overlays to ViewerConfigOptions. Now there are two sets of overlays config: one for annotations and one for content search. By default, I made the content search overlays red, and the annotations overlays blue.

Awesome. @wykhuh I'm on standby and give a swift review once you give me the go ahead.

…ver loads

wykhuh · 2024-05-02T17:39:15Z

@mathewjordan @adamjarling I made the requested changes to the PR. I updated the viewer docs page.

adamjarling

Great work @wykhuh . @mathewjordan is following up with some comments about how we'll move forward. 👍

mathewjordan

@wykhuh This, again, is really great work. We're going to approve and publish this as a release candidate today towards v2.9.0. We'll seek to release that minor version soon. There a few clean up suggestions that @adamjarling and I don't want to burden you with. Thanks for your contribution here!

mathewjordan self-requested a review February 27, 2024 20:38

mathewjordan assigned wykhuh Feb 27, 2024

wykhuh force-pushed the feature/content-search branch 2 times, most recently from 4ee1971 to 298fcd4 Compare March 27, 2024 21:39

wykhuh force-pushed the feature/content-search branch from 2360ba9 to 060cc2a Compare April 18, 2024 18:20

wykhuh force-pushed the feature/content-search branch from 060cc2a to d195c64 Compare April 18, 2024 19:23

mathewjordan reviewed Apr 26, 2024

View reviewed changes

mathewjordan self-requested a review April 26, 2024 15:40

mathewjordan requested changes Apr 26, 2024

View reviewed changes

wykhuh force-pushed the feature/content-search branch from 0c56986 to 67557cb Compare May 1, 2024 19:39

wykhuh added 13 commits May 1, 2024 15:51

add package-lock.json

d6db441

create newspaper doc page

1dfbbac

add api endpoint to create search manifests for newspaper

45aa69e

update newspaper manifest to work with dynamic content search service

e56709e

add method to process search content annotations

b823051

refactor method to process search content annotations to use vault

c6160fe

add iiifContentSearch prop to Viewer to execute search query when Clo…

a91d945

…ver loads

add components to display search results, and zoom and pan

ebc42a7

display message if no content search results

e83cd80

refactor getContentSearchResources to load search url

b6f36fb

add content search form to do new searches

edfd60c

limit the number of search results shown

14636b1

add locale text for content search

27d4417

wykhuh added 10 commits May 1, 2024 15:52

add defaultTab option to informationPanel config

75338d1

simplify iiifContentSearchQuery params

c00f219

add separate over configs for annotations and content search

da3b56a

rename newspaper pages to content-search

fadf4eb

add OSDImageLoaded to viewer context

661f9cf

refactor zoom to target when search result is clicked

5adc4cf

fix setting tab when deafaultTab is set

d310524

fix setting tab when defaultTab is set

5536cb6

fix - zoom to target when search result is clicked

49fc6ff

fix crash if dispatch is not a function

2e763fb

wykhuh force-pushed the feature/content-search branch from 67557cb to 2e763fb Compare May 1, 2024 22:54

wykhuh added 10 commits May 2, 2024 01:45

refactor panToTarget to work with annotations and content search

d2b8db9

update content search zoom level

1eaf1ab

add search button to search form

1f82328

deleted unneeded newspaper manifests

96a4428

update search docs page to return static manifest

792bccc

add content search info to viewer docs

b285392

update informationPanel.defaultTab to work with annotations

e7ad904

update content search docs

c3c2cc1

reset output key in next.config.js

8e607f2

make specs pass

428eb15

wykhuh added 3 commits May 2, 2024 12:57

fix toggling the info panel

ef2a6ce

add manifest with dynamic content search

989bb2e

clean up useEffects for content search

a6dc3e3

mathewjordan requested review from adamjarling and mathewjordan May 10, 2024 19:35

adamjarling approved these changes May 10, 2024

View reviewed changes

mathewjordan approved these changes May 10, 2024

View reviewed changes

mathewjordan merged commit 69c6cf2 into samvera-labs:main May 10, 2024
1 check passed

-  iiifContentSearch="http://localhost:3000/api/newspaper_search/1?q=Berliner"
+  iiifContentSearchQuery={{
+    q: "Berliner",
+  }}

	iiifContentSearch={iiifContentSearch}
	iiifContentSearchQuery={iiifContentSearchQuery}

-  iiifContentSearch?: string;
+  iiifContentSearchQuery?: {
+    q: string;
+  };

		@@ -0,0 +1,61 @@
		import type { NextApiRequest, NextApiResponse } from "next";

Feature/content search #172

Feature/content search #172

Conversation

wykhuh commented Feb 23, 2024 • edited Loading

mathewjordan commented Feb 27, 2024

mathewjordan commented Feb 27, 2024

wykhuh commented Feb 28, 2024 • edited Loading

collectiveaccess commented Feb 28, 2024

wykhuh commented Apr 18, 2024

mathewjordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wykhuh May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewjordan left a comment • edited Loading

Choose a reason for hiding this comment

Differentiate Annotation Overlays somehow?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wykhuh Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wykhuh commented Apr 30, 2024 • edited Loading

Differentiate Annotation Overlays somehow?

mathewjordan commented Apr 30, 2024 • edited Loading

Differentiate Annotation Overlays somehow?

wykhuh commented May 2, 2024

adamjarling left a comment

Choose a reason for hiding this comment

mathewjordan left a comment

Choose a reason for hiding this comment

wykhuh commented Feb 23, 2024 •

edited

Loading

wykhuh commented Feb 28, 2024 •

edited

Loading

wykhuh May 2, 2024 •

edited

Loading

mathewjordan left a comment •

edited

Loading

wykhuh Apr 29, 2024 •

edited

Loading

wykhuh commented Apr 30, 2024 •

edited

Loading

mathewjordan commented Apr 30, 2024 •

edited

Loading