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

Video element overflows on summary pages when screen width is less than 700 px. #132

Closed
HarshKapadia2 opened this issue Jan 20, 2023 · 6 comments · Fixed by #178
Closed
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@HarshKapadia2
Copy link
Member

We add a video to indicate that we forgot to take a picture during a particular CatchUp session, for example in the 74th CatchUp summary.

We haven't added any size restrictions to that element, so maybe that's why it overflows on screens less than 700 px in width. The page becomes horizontally scrollable and can be zoomed out, when both of those should not be required.

This affects the common summary page and the individual summary pages with such a video included (Eg: 74th CatchUp summary).

One thing to remember while assigning a width would be the we need to maintain the aspect ratio of the video. (This might be achieved using the aspect-ratio property, but this is just a suggestion.)

Normal page load (overflow should not exist):

Zoomed out (should not happen):

@HarshKapadia2 HarshKapadia2 added bug Something isn't working good first issue Good for newcomers labels Jan 20, 2023
@mohitgangwani
Copy link
Contributor

Can you please link how the page is getting rendered? I want to see the html tree

@mohitgangwani
Copy link
Contributor

mohitgangwani commented Jul 29, 2023

Proposed solution: an easy fix would be to add width 100% to the styling of the video tag, the video will render according to the size available, taking 100% of it. Tried for multiple devices, it is working.

@mohitgangwani
Copy link
Contributor

Proposed solution: an easy fix would be to add width 100% to the styling of the video tag, the video will render according to the size available, taking 100% of it. Tried for multiple devices, it is working.

Any updates? @HarshKapadia2 @tusharnankani @SirusCodes

@tusharnankani
Copy link
Member

Can you please link how the page is getting rendered? I want to see the html tree

Here:

We add a video to indicate that we forgot to take a picture during a particular CatchUp session, for example in the 74th CatchUp summary.


Proposed solution: an easy fix would be to add width 100% to the styling of the video tag, the video will render according to the size available, taking 100% of it. Tried for multiple devices, it is working.

Would you like to open a Pull Request? Feel free to make the changes in the /summary/static/css

@mohitgangwani
Copy link
Contributor

Can you please link how the page is getting rendered? I want to see the html tree

Here:

We add a video to indicate that we forgot to take a picture during a particular CatchUp session, for example in the 74th CatchUp summary.


Proposed solution: an easy fix would be to add width 100% to the styling of the video tag, the video will render according to the size available, taking 100% of it. Tried for multiple devices, it is working.

Would you like to open a Pull Request? Feel free to make the changes in the /summary/static/css

Alright will do.

mohitgangwani added a commit to mohitgangwani/catchup that referenced this issue Jul 31, 2023
Fixed issue OurTechCommunity#132. The video now takes the 100% available space according to the device width, the box sizing property makes sure that the padding and margin remains intact.
@mohitgangwani
Copy link
Contributor

mohitgangwani commented Jul 31, 2023

I have created the PR. Well sorry for the commit name, I had updated the commit name where it asks again for some reason it didn't show up.

tusharnankani pushed a commit that referenced this issue Aug 1, 2023
* Update summary-style.css

Fixed issue #132. The video now takes the 100% available space according to the device width, the box sizing property makes sure that the padding and margin remains intact.

* Fix: video element overflow on summary pages when screen width is less than 700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants