SDSS-591: Add callout component #170
Conversation
There was a problem hiding this comment.
This is looking good! I left some review notes on the code, and then had this feedback for the design and display itself:
- The spacing between the author and the quote text needs to be larger.
- Only the large quote text should be italicized. Everything else is non-italics.
- The quote text should be a serif font ("Source Serif Pro"). Right now it's sans-serif.
Let me know if you have any questions!
| size: 60 | ||
| placeholder: '' | ||
| third_party_settings: { } | ||
| su_callout_blue_bg: |
There was a problem hiding this comment.
Couple things on this field:
We shouldn't bake the color into the id/machine name.
- Something like
su_callout_bgcolor_altmight make more sense.
If they want more options in the future (or we want to maintain flexibility to add more), this should probably be a drop-down instead of a boolean checkbox. We'd only have two options (right now).
- Name the field something like: su_callout_bgcolor
- Add two options:
- Default|bg_color_default
- Blue|bg_color_option1
We can always re-label a color down the line (for example "Blue" could be changed to "Green"). But we can't change the id/machine_name easily, so using something like "option1" instead of "blue" allows us to maintain flexibility.
| .layout-paragraphs-sdss-two-column.layout-paragraphs-sdss-two-column--offset-50-50 { | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
| .layout-paragraphs-sdss-two-column.layout-paragraphs-sdss-two-column--offset-50-50 { | |
| margin: 0; | |
| } | |
| .layout-paragraphs-sdss-two-column { | |
| &.layout-paragraphs-sdss-two-column--offset-50-50 { | |
| margin: 0; | |
| } | |
| } |
This selector is a bit long. Should we separate it for legibility?
There was a problem hiding this comment.
It isn't a SCSS file so that syntax wouldn't work.
| @@ -32,6 +32,7 @@ | |||
| @import 'login-page/index.scss'; | |||
| @import 'main-navigation/index.scss'; | |||
| @import 'news/index.scss'; | |||
| @import 'newsroom-callout/index.scss'; | |||
There was a problem hiding this comment.
In the other PR, #168 , we're adding a /newsroom directory. Should this be a separate SCSS file in that /newsroom directory, instead of a new component?
Also, the component is called "Callout Quote" and not "Newsroom Callout". Should we create a /components/callout-quote directory with generic styles, and then extend it in the /newsroom SCSS created in the other PR?
| &.layout-paragraphs-sdss-two-column--offset-50-50 { | ||
| .layout__region.layout__region--sidebar { | ||
| > div { | ||
| float: right; // may change this later. |
There was a problem hiding this comment.
What are you thinking of changing this to later?
There was a problem hiding this comment.
I was thinking there'd be a better way than a float. However, since we took out the 2 and 3 col layouts, I will remove this block since it is no longer relevant.
| display: inline; | ||
|
|
||
| &::after { | ||
| content: "\201D"; | ||
| display: inline; |
There was a problem hiding this comment.
Do we need to specify these individually for each block quote size, when there is a generic style above (lines 81-90) that should do this for all 3?
| // Hiding the text from display. | ||
| .su-callout-blue-bg, | ||
| .su-callout-white-bg, | ||
| .su-select-font-sizing { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
These should be hidden in the configuration or the template itself, but not with CSS. I can help with that just let me know if you have questions.
|
|
||
| .callout { | ||
| @include grid-media-min('lg') { | ||
| max-width: 511px; |
There was a problem hiding this comment.
I'm a nitpicker here, but why such a specific pixel value?
|
|
||
| &.blue_bg, | ||
| &.white_bg { | ||
| padding: 4.5rem 11.3rem 4.5rem 11.1rem; |
There was a problem hiding this comment.
I'm seeing this padding applied to the callout no matter the column (one, two, three) -- maybe this could applied more generically?
| @charset "UTF-8"; | ||
|
|
||
| //padding set up based on layout. | ||
| .layout--layout-paragraphs-one-colum { |
There was a problem hiding this comment.
| .layout--layout-paragraphs-one-colum { | |
| .layout--layout-paragraphs-one-column { |
I think this is missing an "n".
...t/profiles/sdss/sdss_profile/themes/sdss_subtheme/src/scss/utilities/mixins/_typography.scss
Outdated
Show resolved
Hide resolved
…ss/utilities/mixins/_typography.scss
…callout-component
…ce-sdssgryphon into SDSS-591--add-callout-component
READY FOR REVIEW
Summary
Review By (Date)
Criticality
Review Tasks
Setup tasks and/or behavior to test
Front End Validation
Backend / Functional Validation
Code
Code security
General
Affected Projects or Products
Associated Issues and/or People
- SDSS-591
Resources