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(algolia-search): allow translating search modal #7666

Merged
merged 9 commits into from
Jul 6, 2022

Conversation

forresst
Copy link
Contributor

@forresst forresst commented Jun 23, 2022

Pre-flight checklist

Motivation

The Algolia Search Modal is not translatable. This should solve the problem.

Test Plan

I'm still trying to figure out how to test the site locally in French... help me

Tested locally:

image

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

#4542

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 23, 2022
@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Jun 23, 2022
@Josh-Cena Josh-Cena changed the title feat: add translations for modal search Algolia feat(algolia-search): allow translating search modal Jun 23, 2022
@Josh-Cena
Copy link
Collaborator

I'm still trying to figure out how to test the site locally in French... help me

Try yarn start -l fr? Default theme translations shouldn't even need to run write-translations.

@@ -221,6 +222,149 @@ function DocSearch({
description: 'The ARIA label and placeholder for search button',
});

const translatedModal: DocSearchTranslations = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we extract all the translations to a separate file, or at least outside the component?

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree 👍 we'll want to reduce the size of this component, and probably later also use some of those translations on the search page (quite old/legacy, but we'll want to refactor/upgrade someday)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Josh-Cena @slorber Agree in principle. It makes sense. How should I implement this? I haven't found any examples in the current code that already do this. Unless I have misunderstood the request... 😜

Copy link
Collaborator

Choose a reason for hiding this comment

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

Um, it just means copying everything to another file like src/theme/searchTranslations.ts, and then importing it as import translations from "@theme/searchTranslations"...

@netlify
Copy link

netlify bot commented Jun 23, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit b79c730
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62c3e0478ad7fa0008f3f336
😎 Deploy Preview https://deploy-preview-7666--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Jun 23, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 85 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 86 🟢 100 🟢 100 🟢 100 🟢 90 Report

Comment on lines 238 to 247
cancelButtonText: translate({
id: 'theme.SearchModal.searchBox.cancelButtonText',
message: 'Cancel',
description: 'The label for search box cancel button',
}),
cancelButtonAriaLabel: translate({
id: 'theme.SearchModal.searchBox.cancelButtonAriaLabel',
message: 'Cancel',
description: 'The ARIA label for search box cancel button',
}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks repetitive to me. What are the odds that the user wants different translations for these labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll change it to one

@forresst
Copy link
Contributor Author

I'm still trying to figure out how to test the site locally in French... help me

Try yarn start -l fr? Default theme translations shouldn't even need to run write-translations.

Thank you. But my test doesn't work, I need to look into the details of why it keeps the default values of DocSearch. It's the Modal part (only) that doesn't translate ..... 👀

@Josh-Cena
Copy link
Collaborator

Did you run yarn build:packages or yarn workspace @docusaurus/theme-algolia-search build first?

@Josh-Cena
Copy link
Collaborator

Type-checking errors because @algolia/autocomplete-preset-algolia is not installed. This only happens now because we never pulled in docsearch's types before. This looks more like a bug on Algolia's side?

@Josh-Cena
Copy link
Collaborator

cc @shortcuts @algolia/autocomplete-core references @algolia/autocomplete-preset-algolia in its type definition but doesn't have it as dependency/peer dependency. Maybe @docsearch/react need to install it, at least.

@shortcuts
Copy link
Contributor

shortcuts commented Jul 1, 2022

Hey, should be fixed in 3.1.1 for the DocSearch part, I've pinged the Autocomplete team for the client-search warning but you shouldn't get an error at least

Thanks for the ping :D

@forresst
Copy link
Contributor Author

forresst commented Jul 5, 2022

@Josh-Cena @shortcuts It looks good now

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Code LGTM

Not sure about the naming for the new theme module, since these are just the modal translations, not translations for the entire plugin, but not a blocker

@slorber
Copy link
Collaborator

slorber commented Jul 6, 2022

LGTM 👍 thanks

@slorber slorber merged commit ae2ba5e into facebook:main Jul 6, 2022
@forresst forresst deleted the translate-modal-algolia branch July 6, 2022 18:02
slorber pushed a commit that referenced this pull request Jul 14, 2022
Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants