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

PR: Add transcripts to videos #181

Merged
merged 7 commits into from
Aug 27, 2020
Merged

Conversation

juanis2112
Copy link
Contributor

@juanis2112 juanis2112 commented Aug 20, 2020

Pull Request Checklist

  • Read and followed this repo's Contributing Guidelines
  • Based your PR on the latest version of the correct branch (master or 3.x)
  • Checked your writing carefully for correct English spelling, grammar, etc
  • Described your changes and the motivation for them below
  • Noted what issue(s) this pull request resolves, creating one if needed

Description of Changes

In this PR I will add all the transcripts to the videos as an scrollable element under a dropdown.

The first 2 commits have the directives necessary to do this along with the first script but once @CAM-Gerlach is done reviewing and formatting all of them I will add the rest.

Let me know what you think about the design. I think I'll try to talk to Isabella for some advice on colors and format.

Issue(s) Resolved

Fixes #183

@ccordoba12 ccordoba12 changed the title Add transcripts to videos in docs PR: Add transcripts to videos in docs Aug 20, 2020
@ccordoba12
Copy link
Member

Congratulations @juanis2112! This looks awesome!

I only have one small comment: I think there's not enough padding between the text and the scrollbar on the right

Selección_040

My suggestions to avoid this are:

  • Don't make the text justified
  • Add this css rule
.scroll.docutils > p.card-text {
    padding-right: 15px;
}

With those two changes things look like this

Selección_042

What do you think?

@ccordoba12
Copy link
Member

ccordoba12 commented Aug 20, 2020

Another option would be to remove the margins inside the scroll div and add some padding instead:

div.scroll {
    height: 250px;
    overflow-x: hidden;
    overflow-y: auto;
    /* text-align: justify; */
   font-size: 0.8em;
   color: #2c2e30;
   margin: -20px;
   padding-top: 20px;
   padding-left: 25px;
   padding-right: 25px;
   padding-bottom: 0px;
}

Selección_043

I think I prefer this solution because it makes the scrollbar perfectly aligned with the border of the dropdown.

@dalthviz
Copy link
Member

Note: This needs a rebase

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Looks great @juanis2112 ! Just a bunch of minor typos and formatting changes, almost all of them my own fault, sorry. The content and display of the PR output all looks good.

doc/_static/custom_styles.css Show resolved Hide resolved
doc/_static/custom_styles.css Outdated Show resolved Hide resolved
doc/_static/custom_styles.css Outdated Show resolved Hide resolved
doc/_static/custom_styles.css Outdated Show resolved Hide resolved
doc/_static/custom_styles.css Outdated Show resolved Hide resolved
doc/first-steps-with-spyder.rst Outdated Show resolved Hide resolved
doc/first-steps-with-spyder.rst Outdated Show resolved Hide resolved
doc/first-steps-with-spyder.rst Outdated Show resolved Hide resolved
doc/working-with-spyder.rst Outdated Show resolved Hide resolved
doc/working-with-spyder.rst Outdated Show resolved Hide resolved
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM on my end, thanks @juanis2112 !

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Fantastic job @juanis2112 and @CAM-Gerlach! This looks really great!

I only have one minor comment. Most of the script of First Steps with Spyder > Getting started is not the inside the div that should contain it:

Selección_068

Other than that, this is ready to me.

@ccordoba12 ccordoba12 changed the title PR: Add transcripts to videos in docs PR: Add transcripts to videos Aug 27, 2020
@CAM-Gerlach
Copy link
Member

@ccordoba12 Fixed, sorry, it was an indentation mistake introduced by one of the minor changes I applied to the transcript text.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks!

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

Successfully merging this pull request may close these issues.

Add script to videos in docs
4 participants