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

[Ready] Add video embeds of course to docs #3542

Merged
merged 13 commits into from
Jan 25, 2024
Merged

[Ready] Add video embeds of course to docs #3542

merged 13 commits into from
Jan 25, 2024

Conversation

stichbury
Copy link
Contributor

@stichbury stichbury commented Jan 22, 2024

Description

Added a sphinx extension to embed youtube video into the docs
Added some video into the early parts of the docs (introduction and spaceflights tutorial)
Take a look at e.g. https://kedro--3542.org.readthedocs.build/en/3542/tutorial/spaceflights_tutorial.html for how I have done it.

Development notes

Rebuilt and inspected but I need to see what this looks like on mobile.

Also @noklam @astrojuanlu are there any videos in particular that you want to include? Please let me know where you want them added? Some are quite difficult to place because the video doesn't totally match up to the way the tutorial explains things (the notebook usage at the start is not linear with the narrative we use in the text for spaceflights). Happy for you to just drop in the videos where you think they best fit -- you'll see the syntax and it's very straightforward. Best to have a consistent "Watch the video" subhead before you include the video too.

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@stichbury stichbury added the Component: Documentation 📄 Issue/PR for markdown and API documentation label Jan 22, 2024
@stichbury stichbury self-assigned this Jan 22, 2024
@stichbury
Copy link
Contributor Author

Hmm, I've just checked this and it's not responsive to mobile (well not to iPhone 13 mobile safari). That's not ideal. The iframe way of doing things I used in 0.17 was better than this because it was a hard-coded width, which I could also do with this extension so I shall adjust for that:

<iframe width="560" height="315" style="max-width: 100%" src="https://www.youtube.com/embed/dRnCovp1GRQ" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>

@stichbury stichbury changed the title Add video embeds of course to docs [Draft] Add video embeds of course to docs Jan 23, 2024
@stichbury stichbury marked this pull request as draft January 23, 2024 07:43
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@stichbury stichbury marked this pull request as ready for review January 23, 2024 10:44
@stichbury stichbury changed the title [Draft] Add video embeds of course to docs [Ready] Add video embeds of course to docs Jan 23, 2024
@stichbury stichbury requested a review from tynandebold January 23, 2024 10:44
@stichbury
Copy link
Contributor Author

Added @tynandebold to reviewer list to confirm that the best way to make including a youtube embed and making it responsive to screen size is to use a % size in the embed. Tynan, I know you're probably not familiar with the sphinx extension itself but I'm assuming using relative width is standard practice in front-end code?

Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me!
image

Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Tynan, I know you're probably not familiar with the sphinx extension itself but I'm assuming using relative width is standard practice in front-end code?

This is correct, or a percentage width, to be precise. It seems like the extension is handling all of that? It looks really good to me.

Approving with one small comment about the 90% width.

Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Left a few comments! Overall I think this is a great start, don't think we have to complicate it much more

@stichbury stichbury enabled auto-merge (squash) January 25, 2024 13:23
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@stichbury stichbury merged commit 0a62ead into main Jan 25, 2024
34 checks passed
@stichbury stichbury deleted the add-video-embeds branch January 25, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Documentation 📄 Issue/PR for markdown and API documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants