Only show audio translation option if available #189
Conversation
Reviewer's Guide by SourceryThis pull request addresses issue #171 by adding a conditional check to the audio translation dropdown in the 'item.vue' file. The dropdown will now only be displayed if more than one language is available. This change ensures that the audio translation option is only shown when applicable, improving the user interface. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding unit tests to verify the AudioTranslationDropdown component's behavior with different language array inputs, including edge cases like empty or null arrays.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -8,7 +8,7 @@ | |||
| reactions-bar(:expanded="true", @expand="activeStageTool = 'reaction'") | |||
| //- reactions-bar(:expanded="activeStageTool === 'reaction'", @expand="activeStageTool = 'reaction'") | |||
| // Added dropdown menu for audio translations near the reactions bar | |||
| AudioTranslationDropdown(:languages="languages", @languageChanged="handleLanguageChange") | |||
| AudioTranslationDropdown(v-if="languages.length > 1", :languages="languages", @languageChanged="handleLanguageChange") | |||
There was a problem hiding this comment.
question: Consider edge cases for languages array
The current condition ensures the dropdown is only shown when there are multiple languages. Have you considered how the component should behave if the languages array is empty? Is the current behavior (not rendering in that case) intentional?
| @@ -8,7 +8,7 @@ | |||
| reactions-bar(:expanded="true", @expand="activeStageTool = 'reaction'") | |||
| //- reactions-bar(:expanded="activeStageTool === 'reaction'", @expand="activeStageTool = 'reaction'") | |||
There was a problem hiding this comment.
suggestion: Remove commented-out code if no longer needed
If this commented-out line is no longer needed, consider removing it to improve code readability. If it's kept for future reference, adding a comment explaining why might be helpful.
| //- reactions-bar(:expanded="activeStageTool === 'reaction'", @expand="activeStageTool = 'reaction'") | |
| // If this line is needed for future reference, please uncomment and use it accordingly. | |
| // reactions-bar(:expanded="activeStageTool === 'reaction'", @expand="activeStageTool = 'reaction'") |
|
This room did not have audio translation
No translation box is showed |
This PR closes/references issue #171 . It does so by check if translation size of video to hide/unhide audio translation box
How has this been tested?
Checklist
Summary by Sourcery
This pull request enhances the user interface by conditionally displaying the audio translation dropdown only when more than one language is available, improving the user experience by hiding unnecessary options.