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: init flexsearch plugin #91

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

svalaskevicius
Copy link

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Provide a description in this PR that addresses what the PR is solving. If this PR is going to solve an existing issue, please reference the issue (e.g. close #123).

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Other

Description

Adds flexsearch plugin

Copy link
Member

@Mister-Hope Mister-Hope left a comment

Choose a reason for hiding this comment

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

some changes are required

plugins/plugin-flexsearch/package.json Outdated Show resolved Hide resolved
plugins/plugin-flexsearch/package.json Outdated Show resolved Hide resolved
plugins/plugin-flexsearch/package.json Outdated Show resolved Hide resolved
plugins/plugin-flexsearch/package.json Outdated Show resolved Hide resolved
plugins/plugin-flexsearch/src/client/styles/search.css Outdated Show resolved Hide resolved
plugins/plugin-flexsearch/src/node/index.ts Outdated Show resolved Hide resolved
plugins/plugin-flexsearch/src/shared/index.ts Outdated Show resolved Hide resolved
plugins/plugin-flexsearch/src/client/index.ts Outdated Show resolved Hide resolved
tsconfig.build.json Outdated Show resolved Hide resolved
@Mister-Hope
Copy link
Member

also please add en docs, I can complete the zh one

@svalaskevicius
Copy link
Author

thanks for the review, I'll try to address when I have a chance later!

@svalaskevicius svalaskevicius force-pushed the add-flexsearch branch 6 times, most recently from 0cc1529 to 6ad2695 Compare March 21, 2024 18:31
@svalaskevicius
Copy link
Author

hi, I think I've addressed your comments :)

I've based the docs from the search plugin (as the whole plugin :) ), and adapted a bit.

@coveralls
Copy link

coveralls commented Mar 22, 2024

Pull Request Test Coverage Report for Build 8458428701

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 50.304%

Totals Coverage Status
Change from base Build 8446088169: 0.0%
Covered Lines: 697
Relevant Lines: 1253

💛 - Coveralls

docs/plugins/flexsearch.md Outdated Show resolved Hide resolved
docs/plugins/flexsearch.md Outdated Show resolved Hide resolved
@svalaskevicius svalaskevicius force-pushed the add-flexsearch branch 3 times, most recently from 33f9b55 to aa9f0b2 Compare March 27, 2024 08:06
@svalaskevicius
Copy link
Author

updated again :)

@svalaskevicius
Copy link
Author

hi, comments (I think) have been addressed, also upgraded flexsearch to 0.7, please can you review again?

thanks

@Mister-Hope
Copy link
Member

Mister-Hope commented Mar 28, 2024

It would be great if you can ensure to make new commits instead of rebase and force push. That makes us hard to review.

Here are some suggestions:

  1. Is it possible to only load search index once the search box is focused? The index could be large on huge docs, and I am worried about the entrance chunk size and script exec time.

  2. The search seems to by sync? So on large site will it stuck the ui? If so is it possible to make it a seperate worker?

  3. Are the ui too simple? Could a popup like docsearch be added?

  4. Could we support extra fields? It would be nice if information like tags and categories could be searched.

The original search plugin only index headings, so we do not need to consider these things, however a enhanced seach plugin could achieve these, and even more like search history, better text extracting and others.

An example could be search-pro

}
`

const prepContent = (html: string): string => {
Copy link
Member

Choose a reason for hiding this comment

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

This implementation is too simple IMO, perhaps you can have a look at search-pro plugin.

@svalaskevicius
Copy link
Author

It would be great if you can ensure to make new commits instead of rebase and force push. That makes us hard to review.

Understood

Here are some suggestions:

1. Is it possible to only load search index once the search box is focused? The index could be large on huge docs, and I am worried about the entrance chunk size and script exec time.

2. The search seems to by sync? So on large site will it stuck the ui? If so is it possible to make it a seperate worker?

3. Are the ui too simple? Could a popup like docsearch be added?

4. Could we support extra fields? It would be nice if information like tags and categories could be searched.

The original search plugin only index headings, so we do not need to consider these things, however a enhanced seach plugin could achieve these, and even more like search history, better text extracting and others.

An example could be search-pro

All these are great ideas, however, I don't think I have time enough to implement them :)

All I wanted is to publish the plugin that I had to maintain separately so I don't need to upgrade it manually every time I upgrade vuepress, but for these changes we'd need to find a proper maintainer of the plugin.

As I see the options are:

  1. document your ideas so any volunteer can pick up, and merge the basic functionality as for small websites it's already better than the default one
  2. not merge this PR for a while and keep it open until either a volunteer appears to take it on or it becomes so out of date that it can be closed without losing much
  3. close this PR, without any hard feelings - reason being that the lack of time on my part does not align with minimal expected feature-set on vuepress side :)

Thanks

@Mister-Hope
Copy link
Member

Don't worry , I can finish these, but will take time.

I am planning to migrate search-pro plugin to this repo first.

@svalaskevicius
Copy link
Author

thank you!

also given this, I'm happy if I'm removed from the author field - given I've only copied and adapted, and you're about to improve it in a serious way :)

all the best! and thanks again for maintaining this project!

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.

4 participants