Add optional 'eyebrow' to Text Introduction module #4004
Conversation
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
virginiacc
changed the title
eyebrow to Text Introduction module
virginiacc
changed the title
eyebrow to Text Introduction module| @@ -29,10 +31,19 @@ | |||
| at the bottom of the molecule. | |||
|
|
|||
| ========================================================================== #} | |||
|
|
|||
| {% if value.eyebrow and value.heading %} | |||
There was a problem hiding this comment.
Should this be {% if value.eyebrow or value.heading %}?
There was a problem hiding this comment.
I think it could be just if value.eyebrow, since the validation ensures that if an eyebrow is present, a heading is required.
| {% if value.heading %} | ||
| <h1>{{ value.heading }}</h1> | ||
| {% endif %} | ||
| {% if value.eyebrow and value.heading %} |
There was a problem hiding this comment.
Should be {% if value.eyebrow or value.heading %}?
|
|
||
| h1, | ||
| .h1 { | ||
| .respond-to-max( @bp-xs-max, { |
There was a problem hiding this comment.
Should .respond-to-max( … enclose the h1, .h1 {?
There was a problem hiding this comment.
I agree with Ans that this would be cleaner. I'd also simplify the selector to
.eyebrow + h1,
.eyebrow + .h1 {There was a problem hiding this comment.
Updated in 357720b
| required=False, | ||
| help_text=('Optional: Adds an H5 eyebrow above H1 heading text. ' | ||
| 'Only use in conjunction with heading.'), | ||
| label="Pre-heading" |
There was a problem hiding this comment.
Should this be single quotes?
There was a problem hiding this comment.
TRIVIAL: Also is space around = sign a style standard?
There was a problem hiding this comment.
Yes, quotes should be single quotes. No, weirdly, lack of space around the equals sign is the standard.
There was a problem hiding this comment.
@anselmbradford Single quotes added in 1cde567 -- thanks for catching that!
For the spacing, I think the convention is that there is space around the equals sign except in keyword arguments.
| ])} | ||
| ) | ||
|
|
||
| return cleaned |
There was a problem hiding this comment.
Props for the custom validation here!
There was a problem hiding this comment.
Thanks for the great example in InfoUnitGroup! 👍
There was a problem hiding this comment.
All praise to @chosak!
| @@ -29,10 +31,19 @@ | |||
| at the bottom of the molecule. | |||
|
|
|||
| ========================================================================== #} | |||
|
|
|||
| {% if value.eyebrow and value.heading %} | |||
There was a problem hiding this comment.
I think it could be just if value.eyebrow, since the validation ensures that if an eyebrow is present, a heading is required.
| @@ -29,10 +31,19 @@ | |||
| at the bottom of the molecule. | |||
|
|
|||
| ========================================================================== #} | |||
|
|
|||
| {% if value.eyebrow and value.heading %} | |||
| <header> | |||
There was a problem hiding this comment.
This header element doesn't seem to be serving any purpose. Removing it on a rendered page with dev tools shows no difference.
Can we remove it? We also would not need the two conditionals, then.
There was a problem hiding this comment.
I'd added the header to associate the eyebrow with the H1, but you're right, it doesn't seem to make a difference. Removed in 357720b
| <header> | ||
| {% endif %} | ||
| {% if value.eyebrow %} | ||
| <div class="h5 eyebrow">{{ value.eyebrow }}</div> |
There was a problem hiding this comment.
What would you think about removing the h5 class and applying it via mixin to .eyebrow?
There was a problem hiding this comment.
That's a good question. Could there ever be a non-H5 eyebrow, or is that styling a fundamental characteristic of an eyebrow in our usage?
There was a problem hiding this comment.
That's also a good question :) Perhaps you could check with your designers for their input?
There was a problem hiding this comment.
I have asked for their feedback on this & will update when I have more info :)
There was a problem hiding this comment.
@Scotchester It sounds like eyebrows will always be H5, so I went with the mixin approach in 844e383 -- thanks for the suggestion!
|
|
||
| h1, | ||
| .h1 { | ||
| .respond-to-max( @bp-xs-max, { |
There was a problem hiding this comment.
I agree with Ans that this would be cleaner. I'd also simplify the selector to
.eyebrow + h1,
.eyebrow + .h1 {| required=False, | ||
| help_text=('Optional: Adds an H5 eyebrow above H1 heading text. ' | ||
| 'Only use in conjunction with heading.'), | ||
| label="Pre-heading" |
There was a problem hiding this comment.
Yes, quotes should be single quotes. No, weirdly, lack of space around the equals sign is the standard.
…resh into text-introduction-eyebrow
| .eyebrow { | ||
| .heading-5(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do you no longer need the media query to modify the margin between the eyebrow and its H1?
There was a problem hiding this comment.
That margin is specifically added between h5/.h5 and h1/.h1 at small screen sizes in cf-base, so using the mixin makes the override unnecessary.
There was a problem hiding this comment.
Ah, great! Merge at will.
| @@ -13,3 +13,6 @@ nav ol ol { | |||
| } | |||
|
|
|||
| // END enhancement for normalize-legacy-addon | |||
|
|
|||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Could we remove these added blank lines, please?
There was a problem hiding this comment.
Removed in 4cfe69b
|
Thanks, @anselmbradford & @Scotchester! |
While migrating the BAH journey content into Wagtail, we opted to use an H5 eyebrow above the H1 header in the Text Introduction module to give users additional information about the current step. We initially entered the eyebrow and heading content in the body field. This PR adds an optional
pre-heading/eyebrowfield to Text Introduction module to standardize the markup and make the eyebrow pattern available elsewhere in the site.Additions
Testing
Screenshots
Admin
Desktop
Mobile
Checklist
Testing checklist
Browsers
Accessibility
Other