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

122 hero #259

Merged
merged 5 commits into from
Nov 2, 2018
Merged

122 hero #259

merged 5 commits into from
Nov 2, 2018

Conversation

sherakama
Copy link
Member

READY FOR REVIEW

Summary

  • Adds a new hero component
  • Experimental youtube option - Needs work.

Needed By (Date)

  • TODAY!

Urgency

  • 3:30 will be here fast

Steps to Test

  1. Check out this branch
  2. Compile styleguide
  3. Review hero component for looks
  4. Review code for style
  5. Review code for approach

Associated Issues and/or People

See Also

@sherakama sherakama self-assigned this Nov 2, 2018
@sherakama sherakama requested a review from JBCSU November 2, 2018 18:09
@sherakama sherakama requested a review from josephgknox November 2, 2018 18:09

{# Hero Image or Video #}
<div class="su-hero__media">
{% if 'su-hero--youtube' in modifier_class %}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is my trick to get the KSS styleguide to show both img and youtube video. KSS does not support multiple data sets for the modifier_classes. Maybe one day it will.

Choose a reason for hiding this comment

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

Nice trick!

'xl': 728px
);

.su-hero--youtube {

Choose a reason for hiding this comment

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

Should there be a specific --youtube option or should this be video? Vimeo, for example, allows for videos to be embedded via an iframe.

@charset "UTF-8";

// Local variable not for changing.
$_su-hero-height: (

Choose a reason for hiding this comment

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

I wonder if the embed-container mixin would be useful here. I can see also how it might not be if the ratio for a video option should match that of the image. The mixin keeps a 16:9 ratio while also applying 100% height and width.

Copy link

@josephgknox josephgknox left a comment

Choose a reason for hiding this comment

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

This looks great! I left two thoughts, none of which should block this from being merged.

Copy link
Collaborator

@JBCSU JBCSU left a comment

Choose a reason for hiding this comment

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

NICE!! Great templating, great re-use of Card!

@JBCSU JBCSU merged commit b8c31a9 into master Nov 2, 2018
@JBCSU JBCSU deleted the 122-hero branch November 2, 2018 19:09
JBCSU added a commit that referenced this pull request Nov 2, 2018
* master:
  122 hero (#259)
  195 added all Stanford approved fonts (#248)
  change class for logo to match new naming conventions. (#257)

# Conflicts:
#	core/css/decanter.css - regenerated using mine
#	core/scss/utilities/mixins/typography/_font-lead.scss - resolved using mine
#	core/scss/utilities/mixins/typography/_h6.scss - deleted
yvonnetangsu added a commit that referenced this pull request Nov 2, 2018
* master:
  reversed order of modular scale type mixin (#260)
  122 hero (#259)
  195 added all Stanford approved fonts (#248)
  change class for logo to match new naming conventions. (#257)
yvonnetangsu added a commit that referenced this pull request Nov 2, 2018
* master:
  reversed order of modular scale type mixin (#260)
  122 hero (#259)
  195 added all Stanford approved fonts (#248)
  change class for logo to match new naming conventions. (#257)
  px to rem (#256)
  219 Modular typography (#250)
  fixup! wip. (#255)
  79 card (#241)
  make color change on hover & focus less abrupt (#253)
yvonnetangsu added a commit that referenced this pull request Nov 7, 2018
* master:
  216 Create site search component (#262)
  266: Update scss file to reference logo.twig and update twig file to … (#267)
  246 homepage (#258)
  reversed order of modular scale type mixin (#260)
  122 hero (#259)
  195 added all Stanford approved fonts (#248)
  change class for logo to match new naming conventions. (#257)
  px to rem (#256)
  219 Modular typography (#250)
  fixup! wip. (#255)
  79 card (#241)
  make color change on hover & focus less abrupt (#253)
  249: Change CTA icon to a variable (#251)
  223 brand bar (#243)

# Conflicts:
#	core/css/decanter.css
#	core/scss/components/index.scss
#	core/templates/components/logo/logo.twig
JBCSU added a commit that referenced this pull request Nov 9, 2018
* master:
  89 browsersync (#272)
  189 Alert Component (#263)
  216 Create site search component (#262)
  266: Update scss file to reference logo.twig and update twig file to … (#267)
  246 homepage (#258)
  reversed order of modular scale type mixin (#260)
  122 hero (#259)

# Conflicts: - all resolved using master
#	core/scss/homepage.md
#	kss/builder/decanter/index.twig
#	kss/builder/decanter/kss-assets/css/kss.css
#	kss/builder/decanter/scss/_home.scss
#	kss/builder/decanter/scss/kss.scss
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.

3 participants