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

Changed iframe object to object video player #6915

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

Cloid
Copy link
Member

@Cloid Cloid commented May 30, 2024

Fixes #6749

What changes did you make?

  • Updated iframe to object video player.

Why did you make the changes (we will use this info to test)?

  • To circumnavigate privacy / video blockers dictated by browsers.

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Before Changes image
After Changes image

@HackforLABot HackforLABot added this to PR Needs review (Automated Column, do not place items here manually) in Project Board May 30, 2024
@ajb176
Copy link
Member

ajb176 commented May 30, 2024

Hey @Cloid

Don't hyperlink the issue number, that's done automatically. The first line should just be: Fixes #6749

Just to clarify, your first line currently is: Fixes [#6749](https://github.com/hackforla/website/issues/6749) when it should just be Fixes #6749

@muninnhugin muninnhugin self-requested a review May 31, 2024 17:40
@muninnhugin
Copy link
Member

Reviewing this PR.
Availability: Friday afternoon, Saturday, Sunday morning, Monday.
ETA: by end of Monday.

Copy link
Member

@muninnhugin muninnhugin left a comment

Choose a reason for hiding this comment

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

Hi @Cloid, great work!
What works:

  • Original issue is linked
  • Appropriate into and from branches
  • Good PR description, visual changes are provided
  • Object implementation works well on my Linux laptop

Concerns:

  • There is a merge conflict with the current version of the website that needs to be resolved before it can be merged (if you need help with this let me know)
  • Video does not fit the screen on mobile view (I tested using Galaxy S20 Ultra on Firefox). Googling 'responsive video object' gets several possible fixes for this issue. Changing object's width to a percentage and change height accordingly seems to work on my end.

Project Board automation moved this from PR Needs review (Automated Column, do not place items here manually) to test-pending-approval (Automated Column, do not place items here manually) Jun 1, 2024
@Thinking-Panda
Copy link
Member

@Cloid Please resolve the merge conflict and add an availability and ETA for the requested changes. Thanks!

@roslynwythe roslynwythe self-requested a review June 26, 2024 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: test-pending-approval (Automated Column, do not place items here manually)
Project Board
  
test-pending-approval (Automated Colu...
Development

Successfully merging this pull request may close these issues.

Refactor the Events page header to replace the iframe element
4 participants