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

Embedded the CV overview video in introduction documentation #358

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

Murdock9803
Copy link
Contributor

@Murdock9803 Murdock9803 commented Dec 9, 2023

Fixes #353

Changes done:

  • Removed the [CV Marketing Video] text.
  • Embedded the CircuitVerse Overview video from youtube in the Introduction page bottom.

Screenshots:

before :
Screenshot 2023-12-09 212551

after :
Screenshot 2023-12-09 211441

Preview Link(s):

https://github.com/CircuitVerse/CircuitVerseDocs/pull/358/files#diff-be8049532e9abd127315370c8ffc90a920275c5fb3d7a4dfe197d0a225fa7946

✅️ By submitting this PR, I have verified the following

  • Checked to see if a similar PR has already been opened 🤔️
  • Reviewed the contributing guidelines 🔍️
  • Sample preview link added (add a link from the checks tab after checks complete)
  • Tried Squashing the commits into one

Copy link

netlify bot commented Dec 9, 2023

Deploy Preview for cv-doc ready!

Name Link
🔨 Latest commit a799d16
🔍 Latest deploy log https://app.netlify.com/sites/cv-doc/deploys/6587f20b65a39200081ab89a
😎 Deploy Preview https://deploy-preview-358--cv-doc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -22,4 +22,6 @@ As the CircuitVerse community grows, educators and students can join the online

Watch the below video for a quick demonstration of the CircuitVerse platform.

[CV Marketing Video]
<div style="position: relative; padding-bottom: 56.25%; height: 0; overflow: hidden; max-width: 100%;">
Copy link
Member

Choose a reason for hiding this comment

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

Change it to <div>

Copy link
Contributor Author

@Murdock9803 Murdock9803 Dec 10, 2023

Choose a reason for hiding this comment

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

Sure @Prerna-0202 ma'am, The styling in the div was to make it look responsive maintaining youtube's aspect ratio 16:9

Actually when I removed all the styling and just converted it to <div> in line 25, The CV video filled all of the page because of position: absolute in line 26.
By removing all that position from line 26 too, The aspect ratio became poor. Below is the accompanying image.


Screenshot 2023-12-10 222845

Should I let it be like this or specify some different height and width? at present both height and width are 100%.
Thanks for the help.

Copy link
Member

Choose a reason for hiding this comment

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

Have you tried applying both of the suggested changes I mentioned? Even after applying both suggestions, is the aspect ratio still not correct?

Copy link
Member

@Prerna-0202 Prerna-0202 left a comment

Choose a reason for hiding this comment

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

Great 🚀

@@ -22,4 +22,6 @@ As the CircuitVerse community grows, educators and students can join the online

Watch the below video for a quick demonstration of the CircuitVerse platform.

[CV Marketing Video]
<div style="position: relative; padding-bottom: 56.25%; height: 0; overflow: hidden; max-width: 100%;">
<iframe style="position: absolute; top: 0; left: 0; width: 100%; height: 100%;" src="https://www.youtube.com/embed/3Df-2Cn_80A" frameborder="0" allowfullscreen></iframe>
Copy link
Member

Choose a reason for hiding this comment

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

Also I think that if we write it like this instead of using inline styling, it will be better.<iframe width="500px" height="400px" src="https://www.youtube.com/embed/3Df-2Cn_80A" frameborder="0" scrolling="no" webkitAllowFullScreen mozAllowFullScreen allowFullScreen></iframe>

@@ -22,4 +22,6 @@ As the CircuitVerse community grows, educators and students can join the online

Watch the below video for a quick demonstration of the CircuitVerse platform.

[CV Marketing Video]
<div style="position: relative; padding-bottom: 56.25%; height: 0; overflow: hidden; max-width: 100%;">
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried applying both of the suggested changes I mentioned? Even after applying both suggestions, is the aspect ratio still not correct?

@Murdock9803
Copy link
Contributor Author

Thank you very much 🚀

@Murdock9803
Copy link
Contributor Author

@tachyons as the changes are approved, please take a look at the PR.

@tachyons tachyons merged commit 7450434 into CircuitVerse:master Jan 4, 2024
6 of 14 checks passed
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.

CV Marketing video is not showing at the end of Chapter 1 Introduction
3 participants