-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add bureau page text & media object, content title/description, and content_block modifier patterns #147
Conversation
.media { | ||
display: block; | ||
|
||
&:before, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf-core has a .u-clearfix
utility you can call here instead: https://cfpb.github.io/cf-core/docs/#clearfix
It's a little different, so let me know if we should rethink the utility class or if it's sufficient.
margin-top: unit(@grid_gutter-width / 2, px); | ||
}); | ||
.respond-to-min(600px { | ||
display:table-cell; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just add some space after the semicolon to these three lines?
and content-related classes to layout.
@@ -0,0 +1,36 @@ | |||
/* Bureau page specific */ | |||
|
|||
.content__max-width { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is bureau specific can you name it something bueau-y? Or move it to layouts.less with a topdoc comment so it can be discovered and reused through the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense. Any suggestions for naming? The purpose is readability, so maybe something like readable-max-width?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How dangerous would it be to set a max-width on all paragraphs and headings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we stick with a class name then some off the top of my head ideas are:
.readable-max
.readable-line-length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more convenient to have max-width applied automatically than have to add a class in lots of places if this is a site-wide readability standard.
It looks like the blog content maxes out at 652px wide, and the line breaks in the bureau landing page design suggests a max-width closer to 620px. Would we need to standardize? Would other elements -- lists, links -- need a max-width too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be in favor of at least testing out a site-wide standard on all text elements. Perhaps something like max-width: 40em
would be a good start?
@virginiacc, one last thing, I promise. Can you add quick Topdoc comments to the following new patterns so they show up in the docs pages?
|
and descriptions, and mobile display classes.
Thank you! |
Add bureau page text & media object, content title/description, and content_block modifier patterns
content_block__padded-top
,content_block__border-top
, andcontent_block__border-bottom
modifiersmedia
objectcontent_title
andcontent_desc
hierarchy classeslist__branded
with CFPB-green bulletsu-show-on-mobile
andu-hide-on-mobile
utility classes