Skip to content
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

Fix prose sibling heading margin selectors #184

Merged
merged 1 commit into from
Feb 2, 2021

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 29, 2021

Why: It's assumed (confirmation needed) that the intention of these selectors was to match headings within .usa-prose following any other content in that prose block. Instead, due to the behavior of & in expanding the full selector ancestry, the generated output was along the lines of...

*+.usa-prose>h2,*+.usa-prose>h3 {
    margin-top: 1.7em
}

This can be interpreted as "a heading within a prose block, where the prose block itself is preceded by any other content".

This is expected to have a visual impact on headings in prose content. Example, from the "Typography" page:

Before After
before after

Given that this styling has existed for quite a while, it may be worth reevaluating if the margins are still desirable.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM I like the snug headers

@zachmargolis
Copy link
Contributor

Oh wait I got before and after mixed up. I'm a fan of the snug headers, but if this is more consistent with something else, I'm OK with that too

@aduth
Copy link
Member Author

aduth commented Jan 29, 2021

Oh wait I got before and after mixed up. I'm a fan of the snug headers, but if this is more consistent with something else, I'm OK with that too

It's more that the previous styles weren't really doing what we probably expected them to. I don't have a strong preference to one or the other, but it felt like the intention was to have the margins. And if we don't want them, we'd at least want to remove the * + & styling.

For context, I encountered it with making changes to the branch at #181 to adapt additional content into the "Images" page, where the heading margins didn't feel quite right in a real scenario:

Current Styles Margins Corrected
current margins corrected

@aduth
Copy link
Member Author

aduth commented Jan 29, 2021

I'd also say it's generally more consistent. You can see the headers look decent on the Brand page:

https://design.login.gov/brand/

They look correct there because the sidebar is technically an element preceding .usa-prose. But definitely not what I'd expect to be a prerequisite for header margins to be applied 😬

The Typography page is a bit of an extreme example, where in the real-world there'd usually be paragraph content between headings.

**Why**: It's expected that the intention of these selectors was to match headings within `.usa-prose` following any other content in that prose block. Instead, due to the behavior of `&` in expanding the full selector ancestry, the generated output was along the lines of...

```
*+.usa-prose>h2,*+.usa-prose>h3 {
    margin-top: 1.7em
}
```

This can be interpreted as "any content, followed by a prose block with a heading".
@aduth aduth force-pushed the aduth-fix-sibling-heading-margins branch from 03a838e to 365ebf8 Compare February 2, 2021 13:55
@aduth aduth merged commit 3fc305b into main Feb 2, 2021
@aduth aduth deleted the aduth-fix-sibling-heading-margins branch February 2, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants