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

search #543

Merged
merged 118 commits into from Feb 14, 2024
Merged

search #543

merged 118 commits into from Feb 14, 2024

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jan 14, 2024

Using minisearch, we create the search index by reading all md contents.

The search is done on the client. The index and the client code are loaded only when the user focuses and starts typing a search query.

closes #166

DONE

  • changes dots into a progress bar
  • bundle minisearch into search.js
  • safe title
  • fix css and JS for Safari
  • remove fuzzy (hollow) indicators?
  • apply design to the search index
  • solve an issue where / and ctrl-K open the search pane on small screen, but it should close once you click on a result
  • trim down the complexity: once you navigate you lose the search results
  • keyboard navigation:
    • Up down arrows move in the results, the first result is already "selected" (press enter);
    • escape clears
  • investigate why black dots become hollow when navigating with the keyboard
  • escape on empty input blurs
  • / (slash) as an alias to cmd-K
  • prevent iframe (theme demos) from stealing the focus
  • Fix title detection logic (use parseMarkdown)
  • Position below the site name
    • remove obsolete min-height
    • nicer styles? magnifier icon, etc, see figma
  • use a reserved name
  • fix opt-in/opt-out as per review;
    • don't index themes
  • unit tests
  • add search box on all pages
  • focus search input on ctrl-K
    • open the sidebar if necessary
  • lazy load the index
  • lazy load client/search.js and minisearch.js
  • update the index live (or with a small debounce delay) when md files are added, removed, or edited
    • or just refresh it on preview, build, or deploy
  • design results list
    • design fuzzy matches
  • make the whole experience less jumpy
    • save results not search term for faster restitution
  • investigate better indexing (could include keywords, etc)? the current strategy seems quite good for the cli documentation, but it doesn't have that many documents.
    • investigate clipping all words to 10 letters; longer words are probably not ambiguous.
  • investigate potential race condition where if you type "da" faster than the index loads I'm not sure if we display the results of "d" or "da"
  • use front-matter, index h1
  • investigate skipping code blocks (unless they use echo?) and inline code
search.mp4

Future enhancements, maybe:

  • make the input sticky so it doesn't scroll out
  • highlight searched term(s) in the page
  • tab should work as arrowDown unless we are at the end of the list / similarly shift-tab (this would require treating the a:focus style overall, not for this PR)
  • add extracts? (difficult and slow); maybe for a dedicated search page, that could also have filters (ie on keywords defined in front-matter), exact vs fuzzy matches, etc.
  • @eagereyes found an issue with the cancel button in an earlier version, still unresolved but let's hope it doesn't creep back

@mbostock
Copy link
Member

This is a neat demo of the power of data loaders! It does feel like the sort of thing you’d want to build into every project, though, not just add it explicitly to a given project. This will be a nice feature to have and I suspect it will be worth it at some point soon!

@Fil Fil force-pushed the fil/minisearch branch 2 times, most recently from 4c1e92d to deab03f Compare January 29, 2024 15:05
@Fil Fil force-pushed the fil/minisearch branch 3 times, most recently from b4f6f31 to ce64331 Compare February 1, 2024 15:42
@Fil Fil mentioned this pull request Feb 1, 2024
src/dataloader.ts Outdated Show resolved Hide resolved
@Fil
Copy link
Contributor Author

Fil commented Feb 14, 2024

100 commits!

@@ -1,4 +1,5 @@
import MiniSearch from "minisearch";
// eslint-disable-next-line import/no-relative-packages
Copy link
Member

@mbostock mbostock Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we should find a way to not need this (meaning a bare import should work and resolve to node_modules). Do we not have the node-resolve plugin already? Or is because we’re calling esbuild directly rather than rollup? I thought we were using rollup and supported this already…

Comment on lines 1 to 2
// eslint-disable-next-line import/no-relative-packages
import MiniSearch from "../../node_modules/minisearch/dist/es/index.js";
Copy link
Contributor Author

@Fil Fil Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do rollup with the node-resolve plugin,
https://github.com/observablehq/framework/blob/main/src/rollup.ts#L41

(I have tried various combinations of resolveOnly—it only ever gets the "src" module. Removing the option altogether doesn't work either.)

the dist/es/index.js is not necessary, using the following works:

Suggested change
// eslint-disable-next-line import/no-relative-packages
import MiniSearch from "../../node_modules/minisearch/dist/es/index.js";
// eslint-disable-next-line import/no-relative-packages
import MiniSearch from "../../node_modules/minisearch";

🤷🏼

@mbostock
Copy link
Member

I think the test broke because the number of uploaded files changed. Investigating.

@mbostock mbostock merged commit 1b7e1f2 into main Feb 14, 2024
2 checks passed
@mbostock mbostock deleted the fil/minisearch branch February 14, 2024 19:50
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

Successfully merging this pull request may close these issues.

Search
2 participants