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.

Sign up for GitHub

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

D8CORE-628: Add caption option to hero banner. #517

Merged
merged 10 commits into from
Oct 25, 2019
Merged

D8CORE-628: Add caption option to hero banner. #517

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b298828
D8CORE-628: Add caption option to hero banner.
sherakama Oct 21, 2019
3b9d620
fixup! cc fixes.
sherakama Oct 21, 2019
4e82f81
Update config.yml
sherakama Oct 21, 2019
22587c7
D8CORE-628: Update hero template example markup.
sherakama Oct 22, 2019
ab95b64
fixup! 60% width for captions on large screen sizes.
sherakama Oct 22, 2019
c76c751
fixup! merge with master.
sherakama Oct 22, 2019
e871a47
Update hero.twig
sherakama Oct 23, 2019
57c399a
Update _hero.scss
sherakama Oct 23, 2019
52c5245
Update _hero--caption.scss
sherakama Oct 23, 2019
a222470
fixup! wip.
sherakama Oct 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 92 additions & 22 deletions core/dist/css/decanter.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 45 additions & 0 deletions core/src/scss/components/hero/_hero--caption.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
@charset "UTF-8";

.su-hero--caption {

// Ensure no card shows up.
.su-hero__card {
display: none;
}

// The media element shall not be absolute in this option.
.su-hero__media {
@include grid-media('md') {
max-height: map-get($_su-hero-height, 'md');
position: relative;
}

@include grid-media('lg') {
max-height: map-get($_su-hero-height, 'lg');
position: relative;
}

@include grid-media('xl') {
max-height: map-get($_su-hero-height, 'xl');
position: relative;
}
}

.su-hero__content {
@include centered-column;
@include modular-spacing(padding-top, 0);
@include modular-spacing(padding-bottom, 2);
text-align: right;
Copy link
Member

Choose a reason for hiding this comment

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

This might be a @kerri-augenstein question. When the caption container isn't full width, it looks good with text-align: right, but I think it looks a bit unnatural at xs to md breakpoints when the caption takes up the full width and the text is still align to the right. Could we make the caption left-aligned or center-aligned at xs to md breakpoints?

display: block;

p {
@include credits;
Copy link
Member

Choose a reason for hiding this comment

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

The caption currently in this PR is larger and darker than it should be, but the credits/caption font sizes and colors are being worked on in my media component PR, so this would fix itself later after that's merged in.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm overriding the style for the <p> element only and it will be up to the user to supply the correct html and styles they want.

Copy link
Member

@yvonnetangsu yvonnetangsu Oct 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, that makes total sense. Just want to document here that the base <p> will look different since @credits is being worked on.

}

// Only allow for 60% width on large screen sizes.
@include grid-media('lg') {
@include padding(null null null 40%);
Copy link
Member

Choose a reason for hiding this comment

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

This 40% padding-left looks larger than 40% because it's 40% of the full browser width (including the screen edges) instead of 40% of the center container width. If the width doesn't have to be exactly 60% then it would probably look close enough with padding-left:30%. This issue is more obvious when the browser is really wide (try resizing to 2000px, the caption gets quite narrow).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah, good catch. I was thinking since it was full width but it really isn't. I have revised.

}

}
}
2 changes: 1 addition & 1 deletion core/src/scss/components/hero/_hero--youtube.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ $_su-hero-height: (
'lg': 640px,
'xl': 728px,
'2xl': 728px
);
) !default;

.su-hero--youtube {

Expand Down
56 changes: 30 additions & 26 deletions core/src/scss/components/hero/_hero.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ $_su-hero-height: (
'lg': 409px,
'xl': 520px,
'2xl': 520px
);
) !default;

//
// Hero
Expand All @@ -17,6 +17,7 @@ $_su-hero-height: (
//
// Experimental: The YouTube player has not been finished.
//
// .su-hero--caption - An under banner caption option.
// .su-hero--youtube - A YouTube video in the background.
//
// Markup: ../templates/components/hero/hero.twig
Expand All @@ -40,6 +41,31 @@ $_su-hero-height: (
@include grid-media('xl') {
min-height: map-get($_su-hero-height, 'xl');
}

// The content
.su-hero__card {
background: $su-color-white;
position: relative;
z-index: 10;

@include grid-media('md') {
@include margin(45px null);
left: 45px;
max-width: 400px;
}

@include grid-media('lg') {
max-width: 450px;
}

@include grid-media('xl') {
@include margin(96px null 0 null);
bottom: 48px;
left: 48px;
max-width: 450px;
top: auto;
Comment on lines +62 to +66
Copy link
Member

Choose a reason for hiding this comment

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

In the future you might want to finetune the position of the card when the banner is full browser width, but that's probably for another PR. For our WordPress template, if the banner is full width, we line up the edge of the box with the edge of the center container, e.g., if the box/card is on the left, its left edge lines up with the left edge of the logo lockup.

Copy link
Member

Choose a reason for hiding this comment

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

FYI this is what we have

Screen Shot 2019-10-23 at 5 30 44 PM

Copy link
Member Author

@sherakama sherakama Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is existing code that was outside its scope. Before, this .su-card class was not nested under the .su-hero class and was affecting cards elsewhere. What you have in the redwood theme looks great.

}
}
}

// The media element.
Expand Down Expand Up @@ -72,31 +98,9 @@ $_su-hero-height: (
object-position: 50% 50%;
width: 100%;
}

}

// The content
.su-hero__card {
background: $su-color-white;
position: relative;
z-index: 10;

@include grid-media('md') {
@include margin(45px null);
left: 45px;
max-width: 400px;
}

@include grid-media('lg') {
max-width: 450px;
}

@include grid-media('xl') {
@include margin(96px null 0 null);
bottom: 48px;
left: 48px;
max-width: 450px;
top: auto;
}

// Mostly for the demo. You really shouldn't put content in both the card and the content variables.
.su-hero__content {
display: none;
}
1 change: 1 addition & 0 deletions core/src/scss/components/hero/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@

@import
'hero',
'hero--caption',
'hero--youtube';
3 changes: 2 additions & 1 deletion core/src/templates/components/hero/hero.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@
"hero_cta_attributes": "",
"hero_cta_label": "CTA Link Label",
"hero_button_link": "https://stanford.edu",
"hero_button_label": "Button Label"
"hero_button_label": "Button Label",
"hero_content": "<p>Lorem ipsum <strong>dolor sit amet</strong>, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud <a href=\"#\">exercitation ullamco laboris nisi</a> ut aliquip (<i>ex ea commodo consequat</i>).</p>"
}
14 changes: 14 additions & 0 deletions core/src/templates/components/hero/hero.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
* - hero_cta_label: The clickable text for the CTA link
* - hero_button_link: The href property of the button component.
* - hero_button_label: The text label of the button component.
* - hero_content: An open area for content to go beneath the media when using
* the su-hero--caption variant.
*/
#}

Expand All @@ -34,6 +36,8 @@
<div{{ attributes }} class="su-hero {{ modifier_class }}">

{# Hero Image or Video #}
{% block heromedia %}
{% if hero_media is not empty or hero_image is not empty %}
<div class="su-hero__media">
{% if 'su-hero--youtube' in modifier_class %}
{# Support for: iframe #}
Expand All @@ -43,7 +47,11 @@
{{ hero_image}}
{% endif %}
</div>
{% endif %}
{% endblock %}

{# Only include the card template if there is content to render. #}
{%- block herocontent -%}
{%- if
hero_super_headline is not empty or
hero_headline is not empty or
Expand All @@ -67,4 +75,10 @@
only
%}
{%- endif -%}
{%- if hero_content is not empty -%}
<div class="su-hero__content">
{{ hero_content }}
</div>
{%- endif -%}
{%- endblock -%}
</div>
Loading