Skip to content

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

@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

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

ha-bach commented May 31, 2024

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

Copy link
Member

@ha-bach ha-bach 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.

@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
@Cloid
Copy link
Member Author

Cloid commented Jul 30, 2024

Hi there, sorry for the delay. I'll finish this by EOD Thursday, 08/01

@Cloid Cloid requested a review from ha-bach August 8, 2024 19:34
@Cloid
Copy link
Member Author

Cloid commented Aug 8, 2024

@muninnhugin Screen sizing is now fixed for all screen sizes, tested via Firefox/Chrome.

@ha-bach
Copy link
Member

ha-bach commented Aug 10, 2024

Reviewing now, should be done by EOD today. Sorry new schedule, I'm really busy during the week now.

Copy link
Member

@ha-bach ha-bach left a comment

Choose a reason for hiding this comment

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

Both mobile screen size and merge conflicts are fixed. Approving this PR. Thanks for contributing @Cloid!

Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

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

Great job @Cloid with this challenging issue.

  • code is clean and complete
  • Pull Request has well-formed branches, screenshots and descriptive text
  • This branch checks out in the browser (Chrome) at all screen sizes
    Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Refactor the Events page header to replace the iframe element

5 participants