-
Notifications
You must be signed in to change notification settings - Fork 3
Style horizontal rules and separators #1575
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
Conversation
🦋 Changeset detectedLatest commit: 6566904 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✔️ Deploy Preview for cloudfour-patterns ready! 🔨 Explore the source changes: 6566904 🔍 Inspect the deploy log: https://app.netlify.com/sites/cloudfour-patterns/deploys/6189a3454205690008f8f6f1 😎 Browse the preview: https://deploy-preview-1575--cloudfour-patterns.netlify.app |
src/base/_defaults.scss
Outdated
hr { | ||
border-color: currentColor; | ||
border-style: solid; | ||
border-width: math.div(size.$edge-medium, 2) 0; |
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 trust you have a good reason, so I'm still going to approve, but perhaps you could add a comment here explaining why this is set to 1/2 of $edge-medium
, rather than just using $edge-small
?
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.
@spaceninja I've pushed up an additional comment that I hope explains it better.
margin-left: auto; | ||
margin-right: auto; |
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.
Is our browser support good enough to use margin-inline
?
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.
@spaceninja Good question. It looks like we currently have 104 occurrences of traditional margin properties in the project already, so I think I'll create a separate issue about investigating this. Maybe also look into a linting error depending on where that lands?
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.
@spaceninja Bam: #1576
Overview
We lacked a default style for
hr
. This changes that. It uses the top border offootnote-group
for basic appearance, but takes more color cues from the left comment border so that it will have slightly better contrast (and better compatibility with secondary themes). (I resisted the urge to style the footnote group to match because that feels like a visual distinction that does not necessarily warrant greater contrast.)It also extends those styles to the WordPress "Separator" block.
Screenshots