-
Notifications
You must be signed in to change notification settings - Fork 23
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
Mobile View of reactions partly off-screen when translation option is active #212
Conversation
Reviewer's Guide by SourceryThis pull request addresses the issue of the mobile view of reactions being partly off-screen when the translation option is active. The changes include adjusting the layout and positioning of the reactions bar and its components, updating the AudioTranslationDropdown component layout, and reordering components in the room view for better UI flow. File-Level Changes
Tips
|
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.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider avoiding the use of
!important
in CSS as it can lead to specificity issues. Try restructuring your selectors for better specificity instead. - The z-index of 9000 seems arbitrarily high. Consider implementing a z-index management system to avoid potential layering issues in the future.
- There's some commented-out code in the diff. If this code is no longer needed, it's best to remove it entirely rather than leaving it commented out.
Here's what I looked at during the review
- 🟡 General issues: 1 issue 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.
@@ -52,21 +52,27 @@ export default { | |||
border-radius: 24px | |||
padding: 4px | |||
transition: transform .3s ease | |||
z-index: 9000 |
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.
suggestion: Consider using a lower z-index value
While it's important to ensure the reactions bar is visible, a z-index of 9000 seems unnecessarily high. Consider using a lower value that still achieves the desired stacking order without potentially conflicting with other high z-index elements.
z-index: 9000 | |
z-index: 100 |
Please share screenshots |
Hi @mariobehling, here the screenshot minimisation/expansion is not really nceessary, is it? -> okay, I will disable expand for reaction bar, make it expanded by default |
Fixes #205
Summary by Sourcery
Fix the issue where the reactions bar is partly off-screen on mobile view when the translation option is active. Enhance the layout and positioning of elements within the ReactionsBar and AudioTranslationDropdown components for better user experience.
Bug Fixes:
Enhancements: