Skip to content

Video embed#36

Merged
TheKitty merged 3 commits into
adafruit:mainfrom
FoamyGuy:video_embed
Nov 27, 2021
Merged

Video embed#36
TheKitty merged 3 commits into
adafruit:mainfrom
FoamyGuy:video_embed

Conversation

@FoamyGuy
Copy link
Copy Markdown
Contributor

resolves: #33

This PR resolves the issue by specifically using an embedded <video> tag for the video rather than relying on github markdown viewer to render the video.

I tested both on github on my branch from this PR, and on a local build of circuitpython.org/awesome page. Both are embedding the video correctly using this technique.

This is a better and more general approach to solve the problem than my first attempt from: adafruit/circuitpython-org#792

Comment thread README.md Outdated


https://user-images.githubusercontent.com/1685947/115119719-d6e21f00-9f77-11eb-84bf-3f7af59948a3.mov
<video style="display: block; width: 720px; margin: auto;" controls src="https://user-images.githubusercontent.com/1685947/115119719-d6e21f00-9f77-11eb-84bf-3f7af59948a3.mov"></video>
Copy link
Copy Markdown
Contributor

@FlantasticDan FlantasticDan Nov 26, 2021

Choose a reason for hiding this comment

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

Adding a hardcoded width: 720px; results in a bit of a broken experience on mobile devices. The following screenshot is a simulation from Chrome DevTools for a Pixel 2 XL:
image

Perhaps change it to be more responsive and do something like max-width: 720px; width: 100%.

A bit unrelated to this pull request, but in the responsive vein on line 2, the Blinka logo could also be made responsive on mobile by adding a style="width: 100%; max-width: 400px" to the <img> tag:
image

@FoamyGuy
Copy link
Copy Markdown
Contributor Author

FoamyGuy commented Nov 26, 2021

Thanks for the review @FlantasticDan, good catch on the width looking broken on mobile. The latest commit changes to use max-width 720px and width: 100%. I tested this change successfully with chrome in a small width window and Android phone with Firefox browser with both the github MD page, and the awesome circuitpython.org page.

I see what you mean about the image at the top as well. In the interest of limiting the PR to one change I'll make a separate PR to apply a similar responsive size fix to the image.

@FoamyGuy
Copy link
Copy Markdown
Contributor Author

@FlantasticDan I tried in a different branch making the image responsive as well, but it seems there is some issue with the way Github renders the image. You're proposed change does fix the image on circuitpython.org/awesome page for mobile layouts. But on the github side the style gets overwritten to max-width 100% even though it's definitely coded to max-width: 400px in the file. Which results in that image getting stretched and a bit fuzzy on larger displays. Tried a few different ways, but am not sure how to overcome that.

@FlantasticDan
Copy link
Copy Markdown
Contributor

@FoamyGuy Thanks for looking into the image business, I thought it would be a one-liner. I looked at the commits you made in that branch on your repo and it seems that GitHub does some unique (and undocumented) markdown -> html generation for their rendered READMEs. Luckily it seems they don't mess with CSS functions, so I've opened #37 to resolve this. If you could review that I'd appreciate it.

@TheKitty
Copy link
Copy Markdown
Collaborator

I merged #37 - are we good on this?

@TheKitty TheKitty merged commit 8f5b278 into adafruit:main Nov 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Video File does not display at https://circuitpython.org/awesome

3 participants