Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add options for custom
lunr
Liquid and JS code #1068add options for custom
lunr
Liquid and JS code #1068Changes from 1 commit
cc9f37e
88313ce
db0f2f8
5ef974d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests site uses
remote_theme: just-the-docs/just-the-docs
, and I think it's equivalent totheme: just-the-docs
with a Gemfile that pulls the gem from GitHub.I find it quite convenient, because I can simply append
@REF
whereREF
can be any tag (release, pre-release, branch, commit) and restart Jekyll, without needing to rerunbundle
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 ofzzzz-search-data.json
which rake makes.An experiment: When I
bundle install
and my Gemfile points togem "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 systemzzzz
file. Therefore, if a user updated the gem containing this PR, then it should have a new localzzzz
file. I am not a Ruby or Gem expert...so these are more logical guesses.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
...then we need to separate sections and two separate numbered lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. "ex" should not be used. "e.g." or "for example" is the clear and correct usage.