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

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

Merged
merged 10 commits into from
Oct 25, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
23 changes: 20 additions & 3 deletions core/dist/css/decanter.css

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

17 changes: 13 additions & 4 deletions core/src/scss/components/hero/_hero--caption.scss
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,24 @@
@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;
clear: both;

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

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%);
}
.su-hero__content-inner {
float: right;

@include grid-media("lg") {
width: ceil($su-screen-lg * 0.6);
}
@include grid-media("xl") {
width: ceil($su-screen-xl * 0.6);
}
@include grid-media("2xl") {
width: ceil($su-screen-2xl * 0.6);
Comment on lines +44 to +51
Copy link
Member

Choose a reason for hiding this comment

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

Are we using fixed width container and not fluid 60%? I'm totally fine with this - looks close enough. Just want to double check because it's a little different from the request.

}
}
}
4 changes: 3 additions & 1 deletion core/src/templates/components/hero/hero.twig
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,9 @@
{%- endif -%}
{%- if hero_content is not empty -%}
<div class="su-hero__content">
{{ hero_content }}
<div class="su-hero__content-inner">
{{ hero_content }}
</div>
</div>
{%- endif -%}
{%- endblock -%}
Expand Down