-
Notifications
You must be signed in to change notification settings - Fork 709
fix: ensure video controls visible on large screens #2326
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
base: master
Are you sure you want to change the base?
fix: ensure video controls visible on large screens #2326
Conversation
Signed-off-by: Mohammed Firdous <[email protected]>
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Newcomers' Guide and sure to join the community Slack. |
❌ Deploy Preview for mesheryio-preview failed.
|
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.
Summary of Changes
Hello @mohammedfirdouss, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses an issue where video controls were not fully visible on larger screens within the catalog "Help" and generic video modals. The changes refactor the modal layouts to use flexbox for centering, ensure they span the full viewport, constrain their height to fit the screen, and enable internal scrolling for content overflow. Additionally, videos are now responsively scaled to guarantee their native controls remain accessible across all screen sizes.
Highlights
- Enhanced Modal Responsiveness and Layout: Modals are now configured with flexbox for perfect centering, occupy the full viewport (100vw x 100vh), include appropriate padding, and manage internal content scrolling. This improves user experience by preventing page background scrolling and ensuring content visibility.
- Optimized Video Scaling: Video elements within modals are updated to scale responsively (width: 100%; height: auto; max-height: 70vh;), ensuring that video controls are always visible and accessible, regardless of screen size or modal content height.
- Refactored CSS for Modals: Significant CSS changes were applied to .help-modal, .help-modal-content in _includes/catalog-help-modal.html and .modal-video, .modal-content-video in _sass/modal.scss to implement the new centering, sizing, and scrolling behaviors.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request effectively addresses the issue of video controls being hidden on large screens by modernizing the modal styles to use flexbox for centering and viewport units for sizing. The changes correctly implement internal scrolling to ensure modal content fits within the screen.
However, I've found a critical issue in both updated modal styles where a display: flex
property overrides a display: none
, which will cause the modals to be permanently visible. I've also included a few medium-severity suggestions to improve CSS maintainability by removing a redundant property, replacing an inline style with a CSS class, and using a class selector instead of an ID selector.
@vr-varad This is ready for a review, but I will check out the suggestions by Gemini Assist as well. |
…r responsiveness Signed-off-by: Mohammed Firdous <[email protected]>
614d45a
to
1cc6d68
Compare
Thank you for your contribution! Add it as an agenda item to the meeting minutes, if you would :) |
Thank you for your contribution! Add it as an agenda item to the meeting minutes, if you would :) |
Any Update on this @mohammedfirdouss |
We can discuss this today on the meeting call @vr-varad |
@arctic-beep, what do you think? |
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.
@mohammedfirdouss
Some comments appear to be redundant.
Also any before and after visuals would help, expedite the review process here.
Any Updates @mohammedfirdouss ? |
Alright @arctic-beep |
I am still on this @vr-varad |
If you have any doubt lets discuss on todays website meet. |
Thanks, it is all good @vr-varad . I hope to complete this before then. Can I message you on slack, DM? @vr-varad |
Thank you for your contribution! Add it as an agenda item to the meeting minutes, if you would :) |
1 similar comment
Thank you for your contribution! Add it as an agenda item to the meeting minutes, if you would :) |
Summary
This PR fixes an issue where video controls were not visible on large screens in the catalog “Help” modal and the generic video modal. The modals are now centered using flexbox, span the full viewport, constrain their height to fit the screen, and scroll internally when content overflows. The video scales responsively so native controls remain visible and accessible on all screen sizes.
Resolves: #1776
What Changed
Modal overlay layout
100vw
x100vh
) with backdropVideo sizing
100%
) with auto heightFiles Touched
modal.scss
catalog-help-modal.html
How to Test
Run locally
Open
http://localhost:4000/catalog
Open the Help modal and play the video
Click “Publish your own best practices”
In the Meshery UI tab, play the video and confirm controls are visible
Verify across sizes: 1366×768, 1920×1080, 2560×1440
Mobile check (~375px width): video scales; controls remain accessible
Expected Behavior
Checklist
Notes for Reviewers
If the video doesn’t play, try opening it directly at
pattern-import.mp4
or remove#t=0.01
from the source. Codec issues can be confirmed via DevTools (NotSupportedError
).Signed commits