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

WIP: adding search to docs #37

Merged
merged 1 commit into from
Oct 14, 2022
Merged

WIP: adding search to docs #37

merged 1 commit into from
Oct 14, 2022

Conversation

karthik2804
Copy link
Contributor

@karthik2804 karthik2804 commented Oct 10, 2022

Still a major work in progress. Proof of concept at this point.

The command to gather the search index from the markdown files is

node md_parser.mjs --dir=./content/ --out=./static/data.json

Then the search functionality can be tested using

npm run spin

PS: The parser is very hacky.

Signed-off-by: karthik Ganeshram karthik.ganeshram@fermyon.com

@radu-matei
Copy link
Member

THIS IS SO PRETTY!

@radu-matei
Copy link
Member

Could we please have the script to generate the index as part of a build script?

@karthik2804
Copy link
Contributor Author

The script is already a part of the PR (md_parser.mjs). I will add it to package.json in a build step. Should the responsibility to build be on the user or should CI build and push it to the branch?

@radu-matei
Copy link
Member

There should be a package.json script that CI calls.

@karthik2804
Copy link
Contributor Author

Will do it. I will have to look up how to build the index in gh action and have it pushed to the branch.

@radu-matei
Copy link
Member

Technically it doesn't need to be checked in. We could generate it just-in-time before we either run locally (when we do npm run spin), or as we deploy the website (so part of the deploy script).

@karthik2804
Copy link
Contributor Author

That makes sense.

@@ -20,7 +20,13 @@
"@fermyon/styleguide": "^0.1",
"@parcel/transformer-sass": "^2.7.0",
"nodemon": "^2.0.20",
"sass": "^1.49.9"
"sass": "^1.49.9",
"args-parser": "^1.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Could you please alphabetically order the imports?
Thanks!

@karthik2804
Copy link
Contributor Author

Updated with cmd/ctrl + K shortcut. It is pretty much ready for review. I think the indexing script needs a little more cleanup but apart from it, things should be in a good place.

@mikkelhegn
Copy link
Member

mikkelhegn commented Oct 11, 2022

The UI and experience are awesome.

What is being indexed? I'm not able to search for quickstart:
image

image

@karthik2804
Copy link
Contributor Author

The UI and experience are awesome.

What is being indexed? I'm not able to search for quickstart:
image

image

Only the content and title of the post are indexed. The word quick start does not seem to appear anywhere in the document. Only the file is named so. I could add an index of file names too to the index.

@mikkelhegn
Copy link
Member

I think what's confusing me is that the heading is showing up initially # quickstart, but not when I search. Do we need to tweak the search? E.g. with a meta field for keywords?

@karthik2804
Copy link
Contributor Author

It shows up initially because the suggested pages is hardcoded. I could look into it and see what I can do.

class ModalSuggest {
constructor() {
this.spinPages = [
el("a.result-subitem", { href: "/spin/quickstart/", onclick: function (e) { searchModal.close() } }, [el("span.result-item-icon", "#"), el("span", "quickstart")]),
Copy link
Member

Choose a reason for hiding this comment

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

"quickstart" should be "Quickstart"

@mikkelhegn
Copy link
Member

I didn't even notice those were hardcoded suggestions :-) But the consistency with that list and what you can search for would be good. Maybe just add the suggestions as search terms?

@karthik2804
Copy link
Contributor Author

I also think it would probably be a good idea to represent the suggested list in a different manner than how search results appear. But I'll look to add those hardcoded strings as part of the index. Any thoughts?

@radu-matei
Copy link
Member

The quickstart debacle is because the title of the page in the sidebar is "quickstart", but the title of the markdown document is "Taking Spin for a spin".

@mikkelhegn
Copy link
Member

I think we need a keywords tag in the meta, which the indexer should parse. That way we can tweak the indexer without changing the look. @tpmccallum you must have good examples of how SEO works on the big Internet :-)

@karthik2804
Copy link
Contributor Author

karthik2804 commented Oct 11, 2022

If there is a keywords field in the frontmatter indexing it is straightforward.

We can add a keywords field in the extra section of the frontmatter. I will update based on if we go down this route. I will also update the suggested post to name it "Taking Spin for a spin" to keep the result consistent.

@karthik2804
Copy link
Contributor Author

karthik2804 commented Oct 11, 2022

Added support for keywords, only added keywords for the quickstart page. There is currently a 10x boost for results from the keyword field so that would allow us to game which results should show up for certain keywords. It would probably be a better idea to add keywords to all the pages on a later PR.

@karthik2804
Copy link
Contributor Author

karthik2804 commented Oct 11, 2022

Added breadcrumbs and resolved #45.

Will work on differentiating the suggested page from the search results next. Can add filters as well to show results from only a certain project.

@mikkelhegn
Copy link
Member

FYI - the npm test has been fixed in upstream main. You can rebase to get it.

@karthik2804
Copy link
Contributor Author

In the last commit

  • Added filters to search based on the project
  • Redesigned Suggest projects to differentiate from search results

ToDo

  • Condense the search index by removing words common words like "a, at, the, then, them, when...." to reduce the size of index that needs to be transferred to the client.

@mikkelhegn
Copy link
Member

Should we merge now and start using it to get more feedback? Then optimize the indexer next week?

@karthik2804
Copy link
Contributor Author

karthik2804 commented Oct 14, 2022

Sounds like a good idea to me. I will rebase

Signed-off-by: karthik Ganeshram <karthik.ganeshram@fermyon.com>
Copy link
Member

@mikkelhegn mikkelhegn left a comment

Choose a reason for hiding this comment

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

Let merge and then get the feedback

@karthik2804 karthik2804 merged commit 8b4239e into fermyon:main Oct 14, 2022
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.

None yet

3 participants