fix(search-algolia): Algolia plugin SearchPage does not respect configuration #10156
Conversation
With this update the SearchPage component now checks the contextualSearch value of the config file. Disjunctive Facet Refinements are no longer added to the query if this setting is false. Additionally if a user has now explicitly defined a title or description in their algolia index config, those items will be used in place of hierarchy items. otherwise hierarchy functions as normal. Tested working for contextualSearch true & false.
|
Hi @ncoughlin! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
slorber
changed the title
There was a problem hiding this comment.
Thanks for trying to solve this issue.
Note that this update converts the file to JS, which I assume is not desirable. This change is based on a JS swizzled version of the component. If you view the change comparison file you should be able to easily modify the original TS file to reflect these changes.
Yes we want to keep TS. I'm not sure what you expect me to do with this PR since I can't merge it as is, nor review it properly considering there's a lot of diff noise that makes it difficult to see the actual significant changes you made.
Am I supposed to fix your PR and restore TS?
Or am I supposed to take inspiration from it and create a new one? 🤷♂️
|
Hey Sebastien,
I didn’t understand enough about TypeScript at the time to properly modify the TS version. However I’m giving myself a crash course right now and I’ll get around to sending a new pull request that properly updates the file in a week or so.
Please just wait a week and I’ll get you a new pull request that resolves this issue.
Regards,
Nick
From: Sébastien Lorber ***@***.***>
Sent: Thursday, May 23, 2024 4:42 AM
To: facebook/docusaurus ***@***.***>
Cc: Nick Coughlin ***@***.***>; Mention ***@***.***>
Subject: Re: [facebook/docusaurus] fix(search-algolia): Algolia plugin SearchPage does not respect configuration (PR #10156)
@slorber requested changes on this pull request.
Thanks for trying to solve this issue.
Note that this update converts the file to JS, which I assume is not desirable. This change is based on a JS swizzled version of the component. If you view the change comparison file you should be able to easily modify the original TS file to reflect these changes.
Yes we want to keep TS. I'm not sure what you expect me to do with this PR since I can't merge it as is, nor review it properly considering there's a lot of diff noise that makes it difficult to see the actual significant changes you made.
Am I supposed to fix your PR and restore TS?
Or am I supposed to take inspiration from it and create a new one? 🤷♂️
—
Reply to this email directly, view it on GitHub <#10156 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYFLVECXPCHVRP3A7Y7PWDZDXIZ5AVCNFSM6AAAAABIAOEC4OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZTG4YDEOJXGI> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AAYFLVAS6W3GZ52JFCDMGQLZDXIZ5A5CNFSM6AAAAABIAOEC4OWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT3TIZDY.gif> Message ID: ***@***.*** ***@***.***> >
|
|
Thanks We'll get a new PR, so let's close this one until then. If you don't send a new PR, feel free to post there a clear JS diff of your solution, I could try to submit the TS PR myself based on your proposed solution. The problem for me atm is that it's even hard to know what you modified exactly |
Note that this update converts the file to JS, which I assume is not desirable. This change is based on a JS swizzled version of the component. If you view the change comparison file you should be able to easily modify the original TS file to reflect these changes.
With this update the SearchPage component now checks the contextualSearch value of the config file. Disjunctive Facet Refinements are no longer added to the query if this setting is false.
Additionally if a user has now explicitly defined a title or description in their algolia index config, those items will be used in place of hierarchy items. otherwise hierarchy functions as normal.
Tested working for contextualSearch true & false.
Pre-flight checklist
Motivation
I wanted the Search page to work. Sounds like this issue has been open for three years. 👍
Test Plan
Was only tested manually with contextualSearch: false/true.
Related issues/PRs
Fix #3805