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

Add tv ad-example styles to SCSS #3812

Merged
merged 3 commits into from
Jun 12, 2020
Merged

Conversation

johnnyporkchops
Copy link
Contributor

@johnnyporkchops johnnyporkchops commented Jun 3, 2020

Summary (required)

  • Remove CSS from any Wagtail html blocks using the TV ad example iframe pattern
  • Add or edit classnames where necessary in html structure in Wagtail
  • Add to _responsive-object.scss and include example html structure there

Resolves #3373

After deployment:
Make changes to these three live pages to reflect the simplified structure (right under 'How to test' below). Save as draft to be published after PR is deployed to production as part of a release(existing pages wont break upon deploying this, we just need to change them after):

Impacted areas of the application

modified: fec/static/scss/components/_responsive-object.scss
Live Wagtail pages using the TV ad example pattern. (listed above)

Screenshots

How to test

<div class="video-frame">
   <div class="iframe-container">
    <iframe src="https://www.youtube.com/embed/REOm_O2VJcc" rel=0 frameborder="0" allowfullscreen></iframe>
   </div>
</div>

NOTE: If you get error on previewing on your local dev servr, you may need to change Wagtail >Settings > Sites > Site > Hostname to localhost. Or you can just publish locally and test that way.


@johnnyporkchops johnnyporkchops changed the title Add tv add example styles to SCSS Add tv ad example styles to SCSS Jun 4, 2020
@johnnyporkchops johnnyporkchops changed the title Add tv ad example styles to SCSS Add tv ad-example styles to SCSS Jun 4, 2020
@johnnyporkchops
Copy link
Contributor Author

For third checklist item above, the new TV example page will likely not go live before this PR is merged, so we only need to change the two pages listed above.

position: relative;

iframe {
height: 62%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we checked this in IE? Last I checked it wanted videos to have heights that were actual numbers and not just percentages, but that's been a while

Copy link
Contributor Author

@johnnyporkchops johnnyporkchops Jun 12, 2020

Choose a reason for hiding this comment

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

@rfultz , Tested and this works in IE. I am basing it loosely on this technique. On a side note, Wagtail used to add to this CSS to draftail video embed as well, but I think I read they removed it and leave it up to us so do in current versions.
https://alistapart.com/article/creating-intrinsic-ratios-for-video/#section4





Copy link
Contributor

Choose a reason for hiding this comment

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

The linter will want two-space indents and exactly one blank line at the bottom.

Should we use quotes inside the url()? I go back & forth

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 go back and forth too. We should decide on one or the other

Copy link
Contributor

@rfultz rfultz 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! I'd like more diversity with the hand in the photo, but that's a different ticket. Just the linting/formatting

@johnnyporkchops
Copy link
Contributor Author

johnnyporkchops commented Jun 12, 2020

@rfultz, I made the spacing changes. If you are good with it, you can merge

@codecov-commenter
Copy link

Codecov Report

Merging #3812 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3812      +/-   ##
===========================================
- Coverage    75.27%   75.25%   -0.02%     
===========================================
  Files          121      121              
  Lines         7352     7352              
  Branches       592      592              
===========================================
- Hits          5534     5533       -1     
- Misses        1818     1819       +1     
Impacted Files Coverage Δ
fec/fec/static/js/modules/calendar.js 91.91% <0.00%> (-0.74%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54182e4...4378d98. Read the comment docs.

@rfultz rfultz merged commit 659c7aa into develop Jun 12, 2020
@rfultz rfultz deleted the feature/css-styles-tv-examples branch June 12, 2020 17:31
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.

CSS Styles for TV ad examples
3 participants