Skip to content
This repository has been archived by the owner on Feb 9, 2022. It is now read-only.

reactivate language support or vue router next #3967

Merged
merged 2 commits into from
Apr 26, 2021

Conversation

posva
Copy link
Contributor

@posva posva commented Apr 15, 2021

Pull request motivation(s)

What is the current behaviour?

If the current behaviour is a bug, please provide all the steps to reproduce and screenshots with context.

What is the expected behaviour?

NB: Do you want to request a feature or report a bug?
NB2: Any other feedback / questions ?

@posva
Copy link
Contributor Author

posva commented Apr 15, 2021

I'm putting this up already to make it faster later on update. This needs to wait for vuejs/vitepress#283 to be released and used by vue router next

@posva posva changed the title reactivate language support reactivate language support or vue router next Apr 15, 2021
@shortcuts
Copy link
Member

hey @posva, should it be merged now?

@posva
Copy link
Contributor Author

posva commented Apr 18, 2021

Not yet! I will ping you this week once we release the fix on vitepress. Thank you 🙏

@posva posva marked this pull request as ready for review April 26, 2021 15:40
@posva
Copy link
Contributor Author

posva commented Apr 26, 2021

@shortcuts The new docs have been deployed with the proper lang attribute and it should work now. Could you merge this, please?

@shortcuts shortcuts merged commit 735d7db into algolia:master Apr 26, 2021
@posva
Copy link
Contributor Author

posva commented Apr 26, 2021

@shortcuts Thank you!
Is there a way to destroy the docsearch instance or reinitialize it when changing languages? I couldn't find it in the docs and docsearch() doesn't seem to return anything

This is the visual effect when searching after switching languages:

Screen Shot 2021-04-26 at 17 54 30

(notice the second algolia search under the first one)

@shortcuts
Copy link
Member

Hi @posva,

I'm not able to reproduce the issue on your website, has it been solved?

We don't provide any way (with JS) to destroy/update the instance, but I'll keep that in mind and try to expose an update API.

A way to solve that would be to create a ref to always keep the language variable up to date

const languageRef = { current: vitepressConfig.langauge }

and pass languageRef.current to your facetFilters

@posva
Copy link
Contributor Author

posva commented Apr 27, 2021

The problem still exists:

In our code we have an update function that removes the HTML (which removes the event listeners attached to the removed HTML) but leaves out the cmd + k listener which adds a new modal anyway:

Screen Shot 2021-04-27 at 10 21 29

I don't understand how you could pass the reference to facetFilters apart from mutating an array but this looks extremely fragile

  const facetFilters = [...]
  docsearch(
    Object.assign({}, userOptions, {
      container: '#docsearch',

      searchParameters: Object.assign({}, userOptions.searchParameters, {
        // pass a custom lang facetFilter to allow multiple language search
        // https://github.com/algolia/docsearch-configs/pull/3942
        facetFilters: facetFilters.concat(
          userOptions.searchParameters?.facetFilters || []
        )
      }),

(I tested it and it works nicely but looks more like a workaround than anything and I'm afraid this could break anytime)

Sometimes we need to unmount the component containing the algolia search and it would be safe to have a destroy/cleanup method from docsearch to remove all event listeners. This will also ensure things work with HMR and any other situation (it's also normal for JS libraries to expose a destroy method to cleanup all of its event listeners)

@shortcuts
Copy link
Member

(it's also normal for JS libraries to expose a destroy method to cleanup all of its event listeners)

This is indeed something that we should provide before the official release of the v3, thanks for the feedback!

Mutating the array seems like a good workaround in the meantime. In case this is something pressing on your side, feel free to open a pull request for a destroy method, we'll happily review it and try to guide you if needed.

posva added a commit to vuejs/vitepress that referenced this pull request Apr 27, 2021
@posva
Copy link
Contributor Author

posva commented Apr 27, 2021

Thanks, this is not really pressing as it still works and is an edge case. But I might give that destroy() method a try

@posva posva deleted the router-next-update branch August 5, 2021 07:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants