Skip to content

docs(windows-rdp): make sure video renders correctly #266

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

Merged
merged 5 commits into from
Jul 10, 2024
Merged

Conversation

matifali
Copy link
Member

@matifali matifali commented Jul 3, 2024

It now renders the video on GitHub but I am not sure if it will on registry.coder.com.

Any way to test it before merging?

@matifali matifali requested review from Parkreiner and stirby July 3, 2024 19:34
@Parkreiner
Copy link
Member

I'm so sorry for not getting to this earlier, Atif. I'll review this first thing tomorrow morning

Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Been poking around this, and I don't think there's a very clear, obvious solution, short of making fixes to the Coder registry's codebase.

The main problem with this PR is that if the markdown is ever put into a context that doesn't understand GitHub-specific syntax, this is all the user sees:

Screenshot 2024-07-10 at 10 08 47 AM

It's not really clear that this is meant to be a video, or that something happens if you click the icon. You can see this if you open the README locally, so I'm going to assume that the Coder registry would have the same problem.

The next thing I tried was embedding full HTML:

<video controls="" muted="" preload="metadata" style="width: 100%; aspect-ratio: auto;">
  <source src="https://github.com/coder/modules/assets/28937484/fb5f4a55-7b69-4550-ab62-301e13a4be02" type="video/webm">

  <p>
    Your browser does not support GitHub's video format.
    <a href="https://github.com/coder/modules/assets/28937484/fb5f4a55-7b69-4550-ab62-301e13a4be02">
      Click here to try downloading the file.
    </a>
  </p>
</video>

This works locally (and I would assume it would work for the Registry, too), but GitHub strips it out completely:
Screenshot 2024-07-10 at 10 13 44 AM


I think a decent compromise for the moment would be to:

  1. Keep the GitHub-specific formatting
  2. Update the image to be a screenshot of the video (I can take care of this later today)
  3. Later on, update the Coder registry code so that it transforms the GitHub video markdown into an actual <video> element

The current code for the markdown seems pretty easy to grasp, since we're letting the marked library take care of the ASTs. I'm hoping an update actually won't be that bad. I'll try to squeeze that in later this week

@matifali
Copy link
Member Author

Thank you for the detailed review and for investigating all options. I will leave this PR open and you can directly push it with the most appropriate solution.

Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Went ahead and made a new thumbnail. Decided to put it in a more local folder, so things have better co-location

@Parkreiner Parkreiner merged commit 352577b into main Jul 10, 2024
2 checks passed
@Parkreiner Parkreiner deleted the render-video branch July 10, 2024 22:56
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.

2 participants