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

fix for #1588 #1589

Merged
merged 2 commits into from Aug 16, 2022
Merged

fix for #1588 #1589

merged 2 commits into from Aug 16, 2022

Conversation

tcoopman
Copy link
Contributor

@tcoopman tcoopman commented Aug 16, 2022

This is a proposal fix for #1588. Hyphens now are found by the index and get a higher score.

These 2 lunr issues are relevant to understand the fix:

Some remarks about the solution:

  • I've exported a __createIndex functions to be able to test it (createIndex uses globals)
  • I've used the searchNodes from the live view docs, which is overkill and can be changed, but before I did, I wanted to make sure that this is the direction you want.
  • I've got no idea on how to build/setup a version of ex_doc and test on the real docs to see how it actually works. If that's something I can easily do, let me know and I'll try it. (so I don't know if I broke anything...)
  • I tried to maintain the coding style of the javascript files, but as there is no formatter config, I'm not sure it's perfect.

@josevalim
Copy link
Member

Hi @tcoopman, awesome job!

Given this is mostly configuration of the search engine, I would say we can go ahead and skip the tests and only focus on the fix. Can you please amend accordingly? Thank you!

@tcoopman
Copy link
Contributor Author

Like this?

@josevalim
Copy link
Member

@tcoopman can you also please follow the style of this PR?

14f0efc

Note that we need to register the function also when loading a serialized index, otherwise the serialized index does not work. :)

@tcoopman
Copy link
Contributor Author

I think it should be good.
One remark the hypenSearch adds the option to search for "phxupdate" now as well. So it should have better results for "phx-update" and "phxupdate".

assets/js/search-page.js Outdated Show resolved Hide resolved
assets/js/search-page.js Outdated Show resolved Hide resolved
@josevalim josevalim merged commit c63e0f4 into elixir-lang:main Aug 16, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

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

Successfully merging this pull request may close these issues.

None yet

2 participants