D8CORE-628: Add caption option to hero banner. #517
Conversation
|
Tugboat has finished building the preview for this pull request! Link: Dashboard: Visual Diffs: |
|
@yvonnetangsu @kerri-augenstein This is ready for your review: https://pr517-8ihnc8ppp7ovm1lh8vxtwt0jr50ixlli.tugboat.qa/item-components-hero.html |
|
@sherakama , is the design for the caption version going into Figma? Is the text for the caption always right-aligned? It seems like you might want to hide the caption in the default for mobile breakpoints |
|
@kerri-augenstein see @yvonnetangsu 's question about Figma. I got the design option from: https://stanfordits.atlassian.net/browse/D8CORE-628 |
|
@yvonnetangsu captions gone on non-caption variant. https://pr517-8ihnc8ppp7ovm1lh8vxtwt0jr50ixlli.tugboat.qa/item-components-hero.html |
| display: block; | ||
|
|
||
| p { | ||
| @include credits; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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%); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Ah yeah, good catch. I was thinking since it was full width but it really isn't. I have revised.
| @include margin(96px null 0 null); | ||
| bottom: 48px; | ||
| left: 48px; | ||
| max-width: 450px; | ||
| top: auto; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @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); |
There was a problem hiding this comment.
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.
| display: block; | ||
|
|
||
| p { | ||
| @include credits; |
There was a problem hiding this comment.
Sure, that makes total sense. Just want to document here that the base <p> will look different since @credits is being worked on.
| @@ -31,15 +31,24 @@ | |||
| @include modular-spacing(padding-bottom, 2); | |||
| text-align: right; | |||
There was a problem hiding this comment.
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?
* master: Use rem instead of em for big button font size so it doesn't blow up in sections if we set a large base font size; finetune big button padding for smaller breakpoints (#522) Change sizes of link icons to use px instead of em to prevent occasional clipping (#523) D8CORE-628: Add caption option to hero banner. (#517) Use percentage for html root font size instead of px so people can scale up using browser font size preferences (#526) Mixins documention update — @centered-column (#448) Adds Tugboat and fixes npm dependencies. (#518)
* master: (28 commits) Updated packages and configuration files. (#527) Use rem instead of em for big button font size so it doesn't blow up in sections if we set a large base font size; finetune big button padding for smaller breakpoints (#522) Change sizes of link icons to use px instead of em to prevent occasional clipping (#523) D8CORE-628: Add caption option to hero banner. (#517) Use percentage for html root font size instead of px so people can scale up using browser font size preferences (#526) Mixins documention update — @centered-column (#448) Adds Tugboat and fixes npm dependencies. (#518) Update _hero.scss (#513) Create Aspect Ratio Mixin (#445) Adding global footer variants and link tweaks for legibility, change layout at MD breakpoint to match Figma (#500) Updated package and lock files. Add parameter to @link-icon to adjust vertical position of icon and different options for $animate parameter (#504) Delete card-2019-08-28.twig (#506) Update _input.scss (#507) Scale down type-a and type-b font size at xs and sm breakpoints, repurpose splash font and minor typography tweaks (#498) Add 4 new @modular-spacing steps (#6 to #9) and modify 2 existing steps (#3 and #4) (#503) add su- prefix to Decanter core and component variables, deprecate old names (#452) Includevar (#496) Put appearance back for select dropdowns (#479) Split input.scss into several files and add new input-base placeholder (#476) ... # Conflicts (resolved manually) # core/dist/css/decanter.css # core/src/scss/components/card/_card.scss # core/src/scss/components/card/index.scss # core/src/scss/utilities/placeholders/components/_card.scss # core/src/templates/components/card/card.twig
READY FOR REVIEW
Summary
Review By (Date)
Urgency
Steps to Test
Affected Projects or Products
Associated Issues and/or People
See Also