-
Notifications
You must be signed in to change notification settings - Fork 398
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(translations): expose Button and Modal translations #1183
Conversation
✔️ Deploy Preview for docsearch ready! 🔨 Explore the source changes: 86ba124 🔍 Inspect the deploy log: https://app.netlify.com/sites/docsearch/deploys/619b868e2ae1d30008ce7bdd 😎 Browse the preview: https://deploy-preview-1183--docsearch.netlify.app |
Oops, tests are not in the CI 🤔 edit: good |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 86ba124:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, works fine on my machine 🚀
minor comments
...props | ||
}: NoResultsScreenProps) { | ||
const { | ||
noResultsText = 'No results for', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translations is hard, and there is chance that the subject will not be placed at the end of the sentence in every language.
(apply for all of them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this spread method does not prevent empty string (but it's an acceptable risk)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translations is hard, and there is chance that the subject will not be placed at the end of the sentence in every language.
Indeed, we might need to provide functions/templates if this become needed in the future
resetButtonTitle = 'Clear the query', | ||
resetButtonAriaLabel = 'Clear the query', | ||
cancelButtonText = 'Cancel', | ||
cancelButtonAriaLabel = 'Cancel', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most of your buttons are not accessible, not sure it's a good idea to start right now with this random component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's still good to have a starting point, this could also potentially give user the opportunity to contribute if they want to help us improve the accessibility, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about that topic unfortunately, I just know that it's not as easy as it seems and it might give the wrong idea that others props might be changeable. (or maybe aria should not even be translated)
I would just note that in open issue if we want to give users the opportunity to help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go
Summary
This PR exposes a new
translation
property to allow translating any raw text or aria-labels of theDocSearch
,DocSearchButton
andDocSearchModal
components.We probably miss some
aria-labels
ortitle
s so don't hesitate to point them outNext steps
Some test cases are context/state based so we need to re-configure Cypress in order to test them.
Motivations
translations
prop to search button #995