New Gallery search for sandcastle #12755
Conversation
|
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
| const jsFile = readFileSync(`${galleryBase}/main.js`, "utf-8"); | ||
| await index.addCustomRecord({ | ||
| url: `?id=${slug}`, | ||
| content: jsFile, |
There was a problem hiding this comment.
Can we surround this with <code> tags for better suggestion display?
| content: jsFile, | |
| content: `<pre><code>${jsFile}</pre></code>`, |
There was a problem hiding this comment.
I've tried out multiple options for this and none preserve the newlines like we'd want to display them directly... This tool is very focused at plain text.
AFAICT when using addCustomRecord it always processes the content as text and escapes html and considers it part of the result.
There is also an addHTMLFile function that lets you pass in a string of html to be processed as html but even then it pulls out the plaintext of the content and seems to strip newlines.
It might be possible to dig into this a bit more but everything I've tried so far doesn't help our unique situation. I think what we have now might be the best we get without a decent bit of extra effort and maybe changes to pagefind itself.
| isNew: boolean; | ||
| }; | ||
|
|
||
| export function GallerySearch({ |
There was a problem hiding this comment.
Can we move this component to its own file?
There was a problem hiding this comment.
I had it that way at first but then it felt excessive. These components are fairly coupled in logic and data flow. I only separated them for encapsulation of logic, specifically the pagefind loading. Also this component is only used inside the larger Gallery component that this file actually exports.
There was a problem hiding this comment.
Hm, hearing that they're tightly coupled seems like a red flag to me. What do you think is the biggest culprit of the coupling? Is it that we're relying on reducers and not contexts to share state?
| <option value={"All"}>All</option> | ||
| {Object.keys(availableFilters.tags).map((label) => ( | ||
| <option value={label} key={label}> | ||
| {label} | ||
| </option> | ||
| ))} |
There was a problem hiding this comment.
I think it may end up being a bit much to iterate on for this particular PR, but I think we discussed some alternatives for how the categories/labels are presented offline.
Could you please add "Refine gallery filter by label presentation" to the list of TODO's?
|
now that #12752 is merged in this will include all sandcastles and demonstrate search better |
|
@ggetz I think this is ready for another look |
|
Overall this is looking good. I do have some concerns about the coupled components, and left a follow up comment above. When testing out the search, it doesn't appear that sandcastle titles are being included in the search. I think they should be. No worries if that's addresses here or in a follow up PR. Just let me know what you'd prefer @jjspace. |
|
Seems like it'll make sense to follow up on this in subsequent PRs, so I'll go ahead and merge this. |
Description
Re-implement searching for sandcastles in the Gallery using pagefind.
Pagefind is primarily targeted at indexing static html files. We need to build our own custom indexes so it can search in the JS code. I've looped this into the script that already builds the gallery list of metadata. I've also chosen to store the build output inside the gallery directory so the FE can be pointed at a single "gallery path" and load everything it needs for search and the list and assets. (Eventually we will need to support multiple galleries but this is not part of this PR)
When
pagefindbuilds an index it also builds the JS module we should import for that index. This makesvite(and TS) a bit upset because it doesn't know where the files are. We need to do a dynamic import and tell vite to ignore it to actually load thepagefindweb module. I encapsulated all this logic in theGallerySearchcomponent to keep it all in one place.Issue number and link
Part of #12566
Testing plan
npm run devin the package ornpm run build-sandcastleandnpm startfrom the project rootIf you want to test with all the sandcastles copy theconvertGallery.jsscript intopackages/sandcastle/scriptsand run it. This will generate all the other sandcastles and re-running the build scripts will index them all.Once Gallery conversion for Sandcastle v2 #12752 is merged I can merge it into this branch and you won't have to do this manually. (I just don't want to mix branches)This branch now has all sandcastlesAuthor checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change