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

improvement: always find a preview to render #2035

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

zachdaniel
Copy link
Contributor

@zachdaniel zachdaniel commented Jan 18, 2025

An unanticipated side effect of using the new search_data for customizing what appears in autocomplete and search is that, unless your search result points at a header (the entire thing we need to avoid), the preview option will show no content.

CleanShot.2025-01-18.at.10.58.56.mp4

I've added a change that I'm hoping will be amenable that improves this. At worst case, it will help in the case of some unexpected structure of the page in normal usage. It only kicks in after the current preview finding logic, which goes to the header in question and grabs the following content.

What this does, is goes up until it finds a header, and uses that, or falls back to the top content. To me this seems like a reasonable solution because the preview is still always a header in the document, not something customizable in search_data for example. I will likely need to do additional reworking of the way that our DSL docs so that the first parent header is a sensible thing to preview for any given DSL option.

CleanShot.2025-01-18.at.10.57.10.mp4

If we can't do this or something like it, then we should revert the entire custom search data feature, because this points out that the only place safe to include in autocomplete into an extra is a header. I really really really hope we don't have to do that though 🙏

Copy link

github-actions bot commented Jan 18, 2025

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

LGTM!

@zachdaniel zachdaniel marked this pull request as ready for review January 18, 2025 16:57
@zachdaniel
Copy link
Contributor Author

In that case, I've marked it ready for review 🙏

@josevalim josevalim merged commit d399df0 into elixir-lang:main Jan 18, 2025
4 checks passed
@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.

2 participants