Skip to content

Conversation

@tylersticka
Copy link
Member

Overview

While providing feedback for @AriannaChau's article prototype, I became aware of two issues with o-rhythm:

  • There was no way to revert to the default rhythm in nested rhythm embeds. So in this PR, I've added the o-rhythm--default modifier for this purpose.
  • The --generous-headings modifier was ineffective on c-heading elements. In this PR, I've added a [class*='heading'] selector to the vertical spacing mixin to serve this purpose in an extensible way.

Screenshots

These screenshots both show an o-rhythm--generous-headings element with a plain h2 and a c-heading of the same level. The spacing before each should be equal.

Before

Screen Shot 2021-08-09 at 1 57 50 PM

After

Screen Shot 2021-08-09 at 1 57 02 PM

@changeset-bot
Copy link

changeset-bot bot commented Aug 9, 2021

🦋 Changeset detected

Latest commit: 1b65b63

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudfour/patterns Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tylersticka tylersticka marked this pull request as ready for review August 9, 2021 21:09
@tylersticka tylersticka requested review from a team and AriannaChau August 9, 2021 21:09
AriannaChau
AriannaChau previously approved these changes Aug 9, 2021
Copy link
Contributor

@AriannaChau AriannaChau left a comment

Choose a reason for hiding this comment

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

It looks good to me.

@tylersticka tylersticka merged commit b20b639 into v-next Aug 9, 2021
@tylersticka tylersticka deleted the fix/o-rhythm-c-heading branch August 9, 2021 22:00
@github-actions github-actions bot mentioned this pull request Aug 9, 2021
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.

4 participants