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

KB: Improve KB search - search by title, description (tags) #3160

Merged
merged 40 commits into from
Feb 5, 2025

Conversation

Blargian
Copy link
Member

@Blargian Blargian commented Jan 28, 2025

Summary

Uses https://github.com/nextapps-de/flexsearch to do the indexing -> gives us more flexibility in future

  • Searches by title and description now
  • implemented new navigation design for knowledge base
  • added breadcrumbs to KB page

Checklist

@Blargian Blargian changed the title KB: Improve KB search KB: Improve KB search - search by title, description (tags) Jan 28, 2025
@gingerwizard
Copy link
Contributor

As discussed:

  • Add All KB articles next to All tags
  • Add All tags (right aligned) to All KB articles

@Blargian
Copy link
Member Author

Blargian commented Feb 3, 2025

Copy link

@crisalbu crisalbu left a comment

Choose a reason for hiding this comment

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

Thansk @Blargian !

the breadcrumbs should look and behave in the same way they behave for other categories, see example below:

Breadcrumbs

@Blargian
Copy link
Member Author

Blargian commented Feb 3, 2025

Thanks @crisalbu, fixed. Here's the latest version: https://clickhouse-docs-kt9adis2h-clickhouse.vercel.app/docs/knowledgebase

@crisalbu
Copy link

crisalbu commented Feb 4, 2025

Thanks @Blargian!

The title shouldn't change to Recently added or Tags in this case, it should stay the same: Knowledge base.
I know, it's a bit weird, because of the button group, but let's implement it like that, please, since that is what we're displaying.

@crisalbu
Copy link

crisalbu commented Feb 4, 2025

Also, can we keep the breadcrumb in the same position?

When we click on a KB article, there's some top margin added to it, not sure why, see example below:

Correct Screenshot 2025-02-04 at 10 23 26
Incorrect Screenshot 2025-02-04 at 10 23 41

@Blargian
Copy link
Member Author

Blargian commented Feb 4, 2025

@crisalbu Title and alignment of the breadcrumbs on the article page fixed, latest changes here: https://clickhouse-docs-plc2wgdl4-clickhouse.vercel.app/docs/knowledgebase

@crisalbu
Copy link

crisalbu commented Feb 4, 2025

Just spotted another small error, there's no space here, and since we're using Knaowledge Base as title, we should also use the same format for the breadcrumb.

Screenshot 2025-02-04 at 11 06 32

@Blargian
Copy link
Member Author

Blargian commented Feb 4, 2025

Just spotted another small error, there's no space here, and since we're using Knaowledge Base as title, we should also use the same format for the breadcrumb.

Screenshot 2025-02-04 at 11 06 32

That's because the breadcrumb uses the page url /docs/knowledgebase which has it as a single word. I'll add something to display it as two words.

@Blargian
Copy link
Member Author

Blargian commented Feb 4, 2025

@crisalbu With breadcrumbs for Knowledge Base as two words: https://clickhouse-docs-1icbcs6a3-clickhouse.vercel.app/docs/knowledgebase

@crisalbu
Copy link

crisalbu commented Feb 4, 2025

Spotted another one:

  • When you search by Tags, the title 2 posts tagged with .... should be displayed under the button group. The title should stay the same, Knowledge Base, until the user clicks on an article, in which case, it should change.
How it was designed Screenshot 2025-02-04 at 11 52 32
How it was implemented Screenshot 2025-02-04 at 11 52 47

Figma design available here

Copy link
Contributor

@gingerwizard gingerwizard left a comment

Choose a reason for hiding this comment

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

minor but some questions

package.json Show resolved Hide resolved
scripts/autogenerate-table-of-contents.sh Outdated Show resolved Hide resolved
src/components/BlogBreadcrumbs/BlogBreadcrumbs.js Outdated Show resolved Hide resolved

[data-theme='light'] .BlogBreadcrumbs .BreadcrumbLink {
color: black;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Copy link
Contributor

Choose a reason for hiding this comment

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

here @Blargian

src/components/ButtonGroup/ButtonGroup.tsx Outdated Show resolved Hide resolved
src/theme/BlogListPage/styles.module.css Outdated Show resolved Hide resolved
src/theme/BlogTagsPostsPage/styles.module.css Outdated Show resolved Hide resolved
scripts/autogenerate-table-of-contents.sh Show resolved Hide resolved
let matchedArticles;
let setMatchedArticles;

if(typeof localStorage !== "undefined") {
Copy link
Contributor

Choose a reason for hiding this comment

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

whats the value here? why do we need to persist results in local storage?

Copy link
Member Author

@Blargian Blargian Feb 4, 2025

Choose a reason for hiding this comment

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

Has to do with how the docusaurus blog components are setup, there is no single component for the sidebar, tags page, the recent page and the article page which renders those components conditionally. As a result when you change urls the sidebar re-renders each time and causes these ugly glitching effects. I'm using local storage to persist the current search between page navigations between the different components.

Copy link
Contributor

@gingerwizard gingerwizard left a comment

Choose a reason for hiding this comment

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

reluctantly accepting the state storage between re-renders, not good alternative

package.json Show resolved Hide resolved

[data-theme='light'] .BlogBreadcrumbs .BreadcrumbLink {
color: black;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

here @Blargian

@@ -0,0 +1,3 @@
.kbTitle{
margin-bottom: 25px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

here

@gingerwizard gingerwizard merged commit 4c51dec into ClickHouse:main Feb 5, 2025
2 of 4 checks passed
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.

3 participants