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

Optionally use material search #202

Merged
merged 5 commits into from
Aug 1, 2024
Merged

Optionally use material search #202

merged 5 commits into from
Aug 1, 2024

Conversation

alexef
Copy link
Contributor

@alexef alexef commented Jul 2, 2024

fixes: #192

see also: backstage/backstage#25497

the search/search_index.json template behaves differently based on which search plugin is being used.

with this change, we allow users to use material theme's search for generating the index.

Differences:

Mkdocs search Material theme search
Document split Both entire document and heading sections Only headings
Content escaping Yes, both title and text No, only titles. Text still contains html tags
Tags support No Yes
Boost support No Yes

the `search/search_index.json` template behaves differently based on which search plugin is being used.

with this change, we allow users to use material theme's search for generating the index.
@alexef
Copy link
Contributor Author

alexef commented Jul 2, 2024

(validated this works as expected in https://github.com/alexef/mkdocs-techdocs-core/blob/dev/src/core.py#L22, but I'm yet to validate that using custom settings like the proposed way in this PR will work)

Copy link
Collaborator

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @alexef, left one comment.

I'm also wondering, would this impact the core Backstage Search feature? Would you be able to test that to confirm?

src/core.py Outdated Show resolved Hide resolved
@alexef
Copy link
Contributor Author

alexef commented Jul 2, 2024

We are using it locally and search seems to work just like before (except that now we don't get duplicate results).

Updated README added a tiny unit test. If you can please have another look.

@alexef alexef requested a review from awanlin July 2, 2024 14:08
Copy link
Collaborator

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Small typo, other changes look good though 👍

README.md Outdated Show resolved Hide resolved
Co-authored-by: Andre Wanlin <[email protected]>
@alexef alexef requested a review from awanlin July 3, 2024 07:28
Copy link
Collaborator

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Thanks again @alexef 🚀

I'm the initial review but we need the TechDocs team to look as well so I'll pull them in. It might take a bit longer as it's a holiday tomorrow in the US.

@awanlin awanlin requested a review from alexlorenzi July 3, 2024 12:31
Copy link
Contributor

@byan1197 byan1197 left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for your contribution!

@tcardonne
Copy link

Hello 👋

First of all thanks for the PR! We also have the need to boost individual pages in a TechDocs but we didn't implement this yet.

We tried using the material-mkdocs search plugin on our side and we had some issue with it.
I'm curious on how you managed to get around these.
With this PR, enabling the flag will result in Backstage search results containing raw HTML tags.

The text contains html tags (like <p> and <code>) which are used for rich text previews on Mkdocs Material.

We've seen also that when using Elasticsearch with Highlighting, the html tags are not taken into account (html tags can be truncated in the search result highlight and highlighting segments conflicts with the html tags).

How are you handling the display of search results in Backstage ?

@alexef
Copy link
Contributor Author

alexef commented Jul 10, 2024

@tcardonne atm I'm using a custom document collator in backstage, which I put in between the default one (that downloads search-index.json from s3) and I strip the tags before they are sent to Opensearch. So my index doesn't contain html tags, and the search results (with highlighting) work just like before.

In backstage/backstage#25497 I'm preparing a less intrusive method that would allow minimal changes to the code, and allow us to continue to use the DefaultTechDocsCollatorFactory collator (by introducing the techdocs transformer, adding tags and stripping html in there).

@alexef
Copy link
Contributor Author

alexef commented Jul 18, 2024

up

@awanlin awanlin merged commit 8894983 into backstage:main Aug 1, 2024
5 checks passed
@alexef alexef deleted the patch-1 branch August 1, 2024 19:19
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.

Duplicate search index entries
4 participants