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

Download link for external videos & style tweaks #31833

Merged
merged 2 commits into from Nov 12, 2019

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Nov 12, 2019

External video levels were a little mis-aligned LP-775 and they didn't have a download video button LP-774. So I added in the button, aligned elements on the page and per request from @fontie715 styled the "Download" button to be more consistent with the "Continue" button. I also added a little test update to ensure the "Download" button appears.

BEFORE:
Screen Shot 2019-11-11 at 5 05 58 PM

AFTER:
Screen Shot 2019-11-11 at 5 29 00 PM

@@ -31,8 +31,12 @@

- if video && use_large_video_player
.clear
.video-container.external-video-container
%a.btn.btn-large.btn-primary.next-stage.submitButton.pull-right= t('continue')
.video-container.external-video-container{{style: 'max-width: 970px; margin: auto'}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer not to use in-line styling, but wasn't sure where I could put this, especially if we don't want it to affect styles of other uses of these elements.

Copy link
Member

Choose a reason for hiding this comment

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

what you have here seems good to me, given that it is a small amount of CSS. the alternative is to put this in application.scss, and scope it by limiting it to elements which match .video-container.external-video-container. breaking up our CSS is something on my list for optimizing our page load times, although it's getting a bit far down the list at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

application.scss is kind of a beast - I see what you mean. Ok, I'll leave this here then.

Copy link
Contributor

@dmcavoy dmcavoy left a comment

Choose a reason for hiding this comment

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

🎉

@Erin007 Erin007 merged commit c1c7d1c into staging Nov 12, 2019
@Erin007 Erin007 deleted the download-link-for-external-videos branch November 12, 2019 16:45
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.

None yet

3 participants