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

Add params to offlineSearch examples #1771

Merged
merged 3 commits into from
Feb 2, 2024
Merged

Add params to offlineSearch examples #1771

merged 3 commits into from
Feb 2, 2024

Conversation

svrnm
Copy link
Contributor

@svrnm svrnm commented Dec 10, 2023

While trying out the offline search feature I struggled to get it working until I figured out that the offlineSearch should be within params.

Copy link

google-cla bot commented Dec 10, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Collaborator

@deining deining left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Your suggestion is valid IMHO, I consider it an improvement to the user guide.
Looking at your changes in search.md, I realized , that offlineSearch: true appears two more times in the subchapters Changing the summary length of the local search results and Changing the maximum result count of the local search. Shouldn't we mention params at these occurrences as well? @svrnm: do you want to go for this as well?

@deining
Copy link
Collaborator

deining commented Dec 10, 2023

Additional note: I just realized that in the chapter documenting how to enable GCSE search engine, the line gcs_engine_id = "xxxx" should be within params as well. We should address this in a separate ticket/PR IMHO.

@svrnm
Copy link
Contributor Author

svrnm commented Dec 11, 2023

Thanks for your contribution! Your suggestion is valid IMHO, I consider it an improvement to the user guide. Looking at your changes in search.md, I realized , that offlineSearch: true appears two more times in the subchapters Changing the summary length of the local search results and Changing the maximum result count of the local search. Shouldn't we mention params at these occurrences as well? @svrnm: do you want to go for this as well?

Yes, I can take a look

@Marzal
Copy link
Contributor

Marzal commented Dec 11, 2023

Additional note: I just realized that in the chapter documenting how to enable GCSE search engine, the line gcs_engine_id = "xxxx" should be within params as well. We should address this in a separate ticket/PR IMHO.

Maybe there should be a full review, because there are more references without the params indications :

@svrnm
Copy link
Contributor Author

svrnm commented Dec 12, 2023

@deining done, I fixed the other 2 places as well

@chalin
Copy link
Collaborator

chalin commented Jan 16, 2024

@deining - is this PR good to go?
@Marzal or @deining - consider creating an issue for the other sections, thanks.

@chalin chalin modified the milestones: 24Q2, 24Q1 Jan 16, 2024
@chalin chalin mentioned this pull request Feb 2, 2024
33 tasks
@chalin
Copy link
Collaborator

chalin commented Feb 2, 2024

@deining - is this ready?

Copy link
Collaborator

@deining deining left a comment

Choose a reason for hiding this comment

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

LGTM

svrnm and others added 3 commits February 2, 2024 08:20
While trying out the offline search feature I struggled to get it working until I figured out that the `offlineSearch` should be within `params`.
Signed-off-by: Severin Neumann <severin.neumann@altmuehlnet.de>
Signed-off-by: Severin Neumann <severin.neumann@altmuehlnet.de>
@chalin chalin merged commit e3d83dc into google:main Feb 2, 2024
11 checks passed
@svrnm
Copy link
Contributor Author

svrnm commented Feb 5, 2024

thanks for merging :-)

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