-
Notifications
You must be signed in to change notification settings - Fork 14
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 sub text to eCR Summary & Document headers #2341
Conversation
@@ -594,7 +614,7 @@ exports[`Snapshot test for EcrLoadingSkeleton should match snapshot 1`] = ` | |||
data-testid="gridContainer" | |||
> | |||
<div | |||
class="grid-row" | |||
class="grid-row margin-bottom-05" |
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.
fyi, I couldn't use gaps because it gets overruled by the display: block
rule
@angelathe would you be able to shift up eCR Summary just slightly so it's in line with the top of the side nav? Also you made the font size for this text correct to the designs (1rem), but that made me realize the font size is wrong everywhere else in the normal body text of the whole viewer (1.06rem). was staring at it for a while wondering if I was seeing things or if the text you added looked small. you think I should add this change as a separate ticket? |
@sarahtress Currently, I'm seeing that the top of the content box is aligned with the top of the side nav, does yours look different? (screenshot attached below) Ooh also about the text sizes, yes, I think that would be better as a separate ticket if that's okay! |
@angelathe sorry I guess this isn't totally in the scope of the ticket either, but in the designs, I add some padding at the top of the side nav so that they look visually aligned: problem is when they're technically aligned, the spacing built into the eCR Summary text makes them not visually look aligned happy to also make this it's own ticket since I think it's more that I'm realizing this issue now, and less that you caused it with this ticket |
@sarahtress Ready for re-review! |
@@ -45,3 +45,6 @@ | |||
width: 25%; | |||
} | |||
|
|||
.padding-top-10px { |
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.
@BobanL Do you know what what this utility class should be named? There wasn't a 10px size when I looked at USWDS, just either 8px (padding-top-1
) or 12px (padding-top-105
)
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 guess we could call it padding-top-1025
OR padding-top-1-25
. I'm in favor of padding-top-1-25
since it's much easier to read.
looks great thanks! |
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.
Looks good to me!
PULL REQUEST
Summary
eCR Summary
andeCR Document
headers (applies to both main page & Loading component)Related Issue
Fixes #2179
Checklist