add options for custom lunr Liquid and JS code
#1068
Conversation
There was a problem hiding this comment.
Hi @diablodale, thanks for taking this on! Apologies for the delay - been swamped with many other PRs and some travel headaches.
I actually think this is a relatively simple and elegant way to do this! That being said, I want to also tag in @pdmosses who's done a great job auditing new features and finding things I've missed.
Some light comments on my mind:
- I think we should make the filepath more like
_includes/lunr/data.json - I'm a bit torn what the "default state" of each of the files should be
- we should document this feature somewhere!
I could see us merging something like this with some more thought. I'm also no expert with lunr, so I can do a bit of research there and give this PR a more thorough review in a few days. Thanks again for submitting this!
File location I have no strong opinion. When we have consensus, I can update and push.
An alternative to the empty files currently in the PR were files with JS and Liquid comments. I took them out in the PR for two reasons
When consensus, I can add comments back in. Maybe the comments are a deep link to the online docs.
Agreed. I can add to |
|
Thanks for the quick response @diablodale! For naming, I'm currently leaning towards:
I agree with your explanation about not necessarily including comments in the includes. Seems good to me! The last thing I think I'd need before giving this a formal review + merge is the docs. If you'd like to draft those up when you're free, that would be great! I don't foresee any hiccups from us getting this in. (@pdmosses is a bit swamped with many other obligations; he's always got great insight on these sorts of things, but if he's not able to get around to it I can give the formal review + merge) |
|
Unfortunately I have no expertise (and very little experience) with JS, so I shouldn't approve such a PR. But when the docs have been added, I could review them for accessibility.
So is @mattxwang! I suggest to request a review from @just-the-docs/reviewers |
No worries. In that case, @diablodale I think we're basically good to go once we rename the files + write the docs. Let me know how I can support you! I can take point on reviewing this PR. |
- add framework for custom content into Lunr indices - update docs for this feature
diablodale
force-pushed
the
lunr-custom
branch
from
0013964
to
cc9f37e
Compare
|
force pushed file rename and added doc |
docs/search.md
Outdated
| 1. When your site used a previous version of Just the Docs, you must update the file | ||
| `assets/js/zzzz-search-data.json` using the [above methods](#generate-search-index-when-used-as-a-gem). |
There was a problem hiding this comment.
| 1. When your site used a previous version of Just the Docs, you must update the file | |
| `assets/js/zzzz-search-data.json` using the [above methods](#generate-search-index-when-used-as-a-gem). | |
| 1. First, ensure that `assets/js/zzzz-search-data.json` is up-to-date; it can be regenerated with `rake` or manually (see: ["Generate search index when used as a gem"](#generate-search-index-when-used-as-a-gem)). |
There was a problem hiding this comment.
@mattxwang I'm a bit puzzled by this bit of the docs update. Surely the sources of a site using JtD as a (remote) theme should not have a copy of the file assets/js/zzzz-search-data.json unless it is already customising search. Please clarify!
There was a problem hiding this comment.
Good question and I have no answer to that 🤷♀️...I have no experience with remote themes. Since it is...remote...I would assume there are no "local" core theme files. Unless they are overriden...if remote themes support that.
There was a problem hiding this comment.
The tests site uses remote_theme: just-the-docs/just-the-docs, and I think it's equivalent to theme: just-the-docs with a Gemfile that pulls the gem from GitHub.
I find it quite convenient, because I can simply append @REF where REF can be any tag (release, pre-release, branch, commit) and restart Jekyll, without needing to rerun bundle.
And yes, the tests site generally relies on the (remote) theme providing everything in _includes, _layouts, and _sass, but it can override them when needed.
There was a problem hiding this comment.
Generally, users of our theme shouldn't have a copy of this; however, in my experience I have seen many, many sites that have copied the file directly into their repo. I had also mostly made this edit originally based off of @diablodale's writing.
(there's some reason that this rake task exists - my understanding is that sometimes, the JSON file doesn't exist. Perhaps I need to do more digging here)
There was a problem hiding this comment.
I found in existing JTD docs https://just-the-docs.github.io/just-the-docs/docs/search/#generate-search-index-when-used-as-a-gem
If you use Just the Docs as a remote theme, you do not need the following steps. If you use the theme as a gem....do rake
When that is not true, that is a separate issue.
I assume for now it is true.
For gem use, rake needs to be run because those "true" instructions tell me to do that. Any zzzz-search-data.json previously created by rake is out-of-date and will not function with this PR -- silently do nothing. This PR needs an updated version of zzzz-search-data.json which rake makes.
An experiment: When I bundle install and my Gemfile points to gem "just-the-docs", github: "diablodale/just-the-docs", branch: "dp-release" it creates a local /usr/local/bundle/bundler/gems/just-the-docs-5d413480c165/assets/js/zzzz-search-data.json. I ?assume? that jekyll will use that local system zzzz file. Therefore, if a user updated the gem containing this PR, then it should have a new local zzzz file. I am not a Ruby or Gem expert...so these are more logical guesses.
There was a problem hiding this comment.
Right - my comment is more that, bundle exec just-the-docs rake search:init shouldn't be necessary, since we bundle the assets folder as part of the gem. Definitely a separate issue, just wanted to provide some context for the above conversation.
docs/search.md
Outdated
| 2. Add a new file named `_includes/lunr/custom-data.json` to your site. Insert your custom Liquid | ||
| code that reads the page object at `include.page` then generates custom Javascript | ||
| fields that hold the custom text you want to index. You can verify the fields you output at | ||
| the generated `assets/js/search-data.json`. |
There was a problem hiding this comment.
| 2. Add a new file named `_includes/lunr/custom-data.json` to your site. Insert your custom Liquid | |
| code that reads the page object at `include.page` then generates custom Javascript | |
| fields that hold the custom text you want to index. You can verify the fields you output at | |
| the generated `assets/js/search-data.json`. | |
| 2. To add Liquid/Jekyll-based data: create a new include at the path `_includes/lunr/custom-data.json`. Insert custom Liquid code that reads various data (ex: `include.page`, `site.data`, `site.static_files`) that then generates valid [JSON](https://www.json.org/json-en.html) to add to the index. Verify the fields in the generated `assets/js/search-data.json`. |
There was a problem hiding this comment.
@mattxwang A very minor meta-nit: "ex" could be read as "excluding", and I don't think we should use it as a short form of "e.g." or "for example".
This seems even more likely to be confusing (at least for European readers) when the colon is omitted, e.g., in "ex front matter, …".
There was a problem hiding this comment.
I'm not confident the recent edits to steps 2 and 3 are clear. As now written, it suggests they are disconnected. "Use 2 for liquid". Use "3 for javascript".
But the truth is the coder must use both. That's how I wrote them tightly together, in sequence, and part of the same numbered list.
This is already a very advanced feature. Anyone using it will know what they are doing. I suspect the majority of these advanced users want to liquid->lunr. And the docs should explain that majority scenario.
If you want to document the ability of two different scenarios:
- liquid->javascript->lunr
- javascript->lunr
...then we need to separate sections and two separate numbered lists.
There was a problem hiding this comment.
@mattxwang A very minor meta-nit: "ex" could be read as "excluding", and I don't think we should use it as a short form of "e.g." or "for example".
This seems even more likely to be confusing (at least for European readers) when the colon is omitted, e.g., in "ex front matter, …".
Agreed. "ex" should not be used. "e.g." or "for example" is the clear and correct usage.
mattxwang
changed the title
lunr Liquid and JS code
|
Yikes, it wasn't ready to be merged. We still have some edits in the docs. |
|
AFAIK @mattxwang doesn't approve of adding new commits to a merged PR, so a new PR would indeed be needed. I guess he merged this (too) quickly to save you further work… |
|
Ah! So sorry; I hadn't realized there were further edits desired. I apologize for the lack of clarity on my edits to the docs. @diablodale or @pdmosses feel free to open a new PR with more docs updates - I agree that a separation of two pipelines (js -> lunr or liquid -> js -> lunr) would be more clear. Since this is my mistake, I'm also happy to take this up if we'd prefer. |
|
I'll open a new PR. @mattxwang, what scenario for js->lunr do you have? I have no example of which I can think. |
Given that you were the driver for this feature, we can also choose to just emphasize For context, my motivations were:
|
|
I suggest we don't document the two motivations you list. The reason for my suggestion is that...
I'll make a PR by end-of-day thursday that merges some of your doc edits into my original PR's doc. |
Sounds reasonable to me! |
This is a prototype for review and discussion. My use and testing of this PR is on top of 6d9d413. The changes are trival to rebase to
mainand I'm happy to do so if this prototype moves forward.To use the prototype, the two include files need to be customized. Here are mine from the draft website diablodale/dp.docs@9c0d836