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

feat: add search to the new documentation website #7878

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

tkajtoch
Copy link
Member

@tkajtoch tkajtoch commented Jul 8, 2024

Summary

This resolves #7877 by integrating docusaurus-lunr-search and adding a data attribute to inform it what content to index.

QA

  • Open PR preview and confirm the search bar is visible in the top nav bar
  • Search by button and confirm there are multiple search results pointing to different documentation sections, for example: Button, Empty button, Split buttons, Icon buttons

@tkajtoch tkajtoch self-assigned this Jul 8, 2024
@tkajtoch tkajtoch requested a review from a team as a code owner July 8, 2024 15:46
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

I see the search includes algolia in it's ids here 😅 I guess in theory we could eject it and update? Not sure how spaghetti this might be 🤔

Screenshot 2024-07-10 at 11 34 23

Screenshot 2024-07-10 at 11 32 46

❓ Do we have an idea about the styling of the search popover? Maybe we could at least align colors? This could be a separate task?

@@ -19,12 +19,12 @@ const getContentStyles = ({ euiTheme }: UseEuiTheme) => {
};

/* OriginalContent holds the document title and markdown content
NOTE: ejecting this results in an error due to using useDoc() hook outside of DocProvider
NOTE: ejecting this results in an error due to using useDoc() hook outside of DocProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ It'll be ejected in this PR.
To prevent the error around useDoc we need to eject the entire related component to ensure same context/providers. 🙈

@tkajtoch
Copy link
Member Author

@mgadewoll I considered ejecting it, but it's pretty complicated, and I wanted to avoid it in this PR. We will improve it later but now the goal is to just have local search

@mgadewoll
Copy link
Contributor

@mgadewoll I considered ejecting it, but it's pretty complicated, and I wanted to avoid it in this PR. We will improve it later but now the goal is to just have local search

Just to clarify, you mean because of the algolia references?
I'm fine to move ahead with it as is for now, but yeah I think we might want to clean this up if the initial idea was not to use it in the first place 😅
I had a quick look at the code yesterday too, and yes it didn't seem trivial to change. There are some wrappers with those ids added, that I did not yet understand where they are coming from 🫠

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ This plugin is a nice initial setup! I think we could even extend the search capabilities when ejecting the components 🤔 (EuiButton vs Button for example) But we can iterate as needed 👍

After the content updates are merged, you'll need to rebase to the ejected DocItem contents. Let me know if there is any trouble!

@tkajtoch tkajtoch force-pushed the feat/add-docs-search branch from 299bbc0 to 14b99bb Compare July 15, 2024 15:59
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @tkajtoch

@tkajtoch tkajtoch merged commit d405f1a into elastic:main Jul 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New docs] Setup local search
3 participants