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

Frontend multi-language support #1690 #1790

Merged
merged 31 commits into from
Aug 30, 2024
Merged

Conversation

bnodir
Copy link
Contributor

@bnodir bnodir commented Jul 7, 2024

Purpose

This PR is to add multi-language support to the frontend of the app, which was previously only available in English. This new feature, along with i18next, will enable the app to automatically adapt to the user's browser language, thereby improving user's experience.

Does this introduce a breaking change?

When developers merge from main and run the server, azd up, or azd deploy, will this produce an error?
If you're not sure, try it out on an old environment.

[ ] Yes
[x] No

Does this require changes to learn.microsoft.com docs?

This repository is referenced by this tutorial
which includes deployment, settings and usage instructions. If text or screenshot need to change in the tutorial,
check the box below and notify the tutorial author. A Microsoft employee can do this for you if you're an external contributor.

[x] Yes
[ ] No

Type of change

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

Code quality checklist

  • The current tests all pass
  • I added tests that prove my fix is effective or that my feature works
  • I ran python -m pytest --cov to verify 100% coverage of added lines
  • I ran python -m mypy to check for type errors
  • I either used the pre-commit hooks or ran ruff and black manually on my code.

See CONTRIBUTING.md for more details.

en es fr jp

@pamelafox
Copy link
Collaborator

Very cool, thanks for the PR!! I won't have any bandwidth to review PRs this week as we've got some live streams happening, but I'll take a look next week.

@bnodir
Copy link
Contributor Author

bnodir commented Jul 15, 2024

@microsoft-github-policy-service agree company="NEC Corporation"

@bnodir
Copy link
Contributor Author

bnodir commented Jul 16, 2024

Please review this PR as it is nearly complete. I will make any necessary revisions based on your feedback. Thank you.

@pamelafox pamelafox changed the title WIP: Frontend multi-language support #1690 Frontend multi-language support #1690 Jul 16, 2024
@pamelafox
Copy link
Collaborator

Okay, I will review this week, thanks for letting me know it's ready!

@pamelafox
Copy link
Collaborator

@bnodir This is so cool! I pushed one change to move the dropdown to a fluentUI dropdown and add a label, which makes the UI more consistent and fixes an accessibility warning.

We need to make the language picker optional, however, as not all developers will be able/willing to support translating all their strings, and some developers are targeting a single language. I think the way to do that is the same way we decide whether to show the vision options- modify the /config route to pass down a "showLanguagePicker" option, add an azd environment variable like ENABLE_LANGUAGE_PICKER, and set that accordingly.
Let me know if you need help implementing that.

Also, if you can, please compare the build times before this change and after this change. We always want to do that when adding new npm packages, so that we're aware of the performance hit of adding a particular package.

@pamelafox
Copy link
Collaborator

@zedhaque I think you were looking into i18n, potentially, for your audience? Am curious if you have any feedback on this PR. Also if you foresee issues with responsiveness.

@bnodir
Copy link
Contributor Author

bnodir commented Jul 22, 2024

@pamelafox Thank you for reviewing the PR and providing your suggestions! I will work on your feedback this week and let you know once I am finished. Meanwhile, could I kindly ask you to have a native speaker review the translations, as they were generated by AI?

Regarding making the language picker optional and comparing build times, I will work on implementing the approach you suggested and may reach out if help is needed.

@pamelafox
Copy link
Collaborator

@bnodir Sure, I will get a native reviewer for translations, thanks for letting me know they were AI generated.

Copy link

Check Broken URLs

We have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue.

Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues
./samples/chat/README.md 1. https://aka.ms/entgptsearchblog

@zedhaque
Copy link
Contributor

@bnodir - just tried your branch. great work thank you!

A quick request - I could use the language value in the API payload. If possible.
Many thanks.

@zedhaque
Copy link
Contributor

One observation - When I enabled "Speech Out" on the chat response page and pressed the "sound" button, the text-to-speech function works depending on the answer language. However, if I went back, changed to another language, and tried again, the speech output remained as the last spoken one. The text-to-speech output remains cached in the previous language after changing the language setting. It only updates to the new language after continuing with further chat interactions.

@bnodir
Copy link
Contributor Author

bnodir commented Aug 16, 2024

@zedhaque - Thank you for your feedback and kind words.

Regarding your request to include the language value in the API payload, could you please clarify it a little more? Is this intended to be used as a value for the search query, instead of the configuration instructions provided below?

docs/deploy_existing.md
You can also customize the search service (new or existing) for non-English searches:

  1. To configure the language of the search query to a value other than "en-US", run azd env set AZURE_SEARCH_QUERY_LANGUAGE {Name of query language}. (See other possible values)

As for your observation, I checked it several times but was unable to reproduce it on my side. Could you please provide more details or steps to help me understand the issue better?

@zedhaque
Copy link
Contributor

@bnodir - I haven't had a chance to try your latest changes yet. I will test the latest repo and get back to you regarding the "speech out" observation.

Regarding the API payload, I was referring to passing the language value that the user selects via overrides (see the attached screenshot). You can see how other values from the developer settings are being populated. This would allow me to use the selected language value in the backend, as it will be available via overrides. Many thanks for looking into this.

Screenshot 2024-08-19 at 4 02 59 PM

@bnodir
Copy link
Contributor Author

bnodir commented Aug 19, 2024

@zedhaque - Thank you for the clarification. Could you please check the attached screenshot to confirm whether the payload request looks correct? If you have any specific adjustments or suggestions, please let me know.
image

@zedhaque
Copy link
Contributor

@bnodir - many thanks 🙏 yes exactly what I wanted. I come back to you tomorrow on the speech out item.

@zedhaque
Copy link
Contributor

@bnodir - Confirming that the issue with "speech out" is not related to your code. It occurs both in the latest repo and in your repo.

@john0isaac - FYI as you are working on #1894 - please see the attached video. The issues are:

  1. If you click "speech out" and don't finish or click the "speech out" button again to stop the audio, then click "Clear chat," the "speech out" should stop. Should this happen on any event or only specific clicks?
  2. Once you play "speech out" and go back to try another example, the speech out still plays the last cached audio snippet.
  3. Interestingly, I have only enabled English speaker, but it also works for other languages (requires chat page reload to clear the previous audio) . I'm not sure why.

Let me know if you'd like me to create a bug report.

speech-out.mp4

@john0isaac
Copy link
Contributor

john0isaac commented Aug 20, 2024

@zedhaque
Yeah, all of the issues you mentioned are already fixed in the PR.
See my comment here:
#1894 (comment)

I did say that they are out of the scope of the PR but went ahead and fixed all of them anyway.
Currently waiting for a review from a maintainer.

@pamelafox
Copy link
Collaborator

Okay, so is this PR ready for re-review, if all the commented issues are addressed?
And I'll also take a look at John's.

@bnodir
Copy link
Contributor Author

bnodir commented Aug 20, 2024

@pamelafox - Yes, all the commented issues have been addressed, and it's ready for re-review. Kindly requesting your feedback.

@pamelafox
Copy link
Collaborator

I'd like to reduce the existing JS size before bringing this in, since this adds ~1MB of JS. I have a PR for that here: #1947

Otherwise, this PR is looking very good. I'll resolve the merge conflicts.

Copy link
Collaborator

@pamelafox pamelafox left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for your patience on the review.

@pamelafox pamelafox merged commit 9073b65 into Azure-Samples:main Aug 30, 2024
17 checks passed
@bnodir
Copy link
Contributor Author

bnodir commented Aug 31, 2024

Thank you so much!

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.

6 participants