-
Notifications
You must be signed in to change notification settings - Fork 3
Update quote styles #1556
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
Update quote styles #1556
Conversation
🦋 Changeset detectedLatest commit: 7bf2834 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: 7bf2834 🔍 Inspect the deploy log: https://app.netlify.com/sites/cloudfour-patterns/deploys/6157758bd0510400082f11da 😎 Browse the preview: https://deploy-preview-1556--cloudfour-patterns.netlify.app |
.c-comment__content blockquote { | ||
border-left: size.$edge-medium solid color.$base-gray-light; | ||
color: color.$base-gray-dark; | ||
} | ||
|
||
.c-comment__content blockquote cite { | ||
font-style: normal; | ||
} | ||
|
||
.c-comment__content blockquote cite::before { | ||
content: '\2014'; | ||
} |
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.
Since these styles are repeated here and in our WordPress vendor styles, should we create a Sass mixin and/or design tokens for them?
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.
That's a good point, I think tokens would work well here.
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.
@tylersticka I added tokens for most of these styles. However, I'm having trouble using a token for font-style: normal
. I added a token in tokens/font/styles.json
, but I get an error when I try to use it.
src/components/comment/comment.scss
Outdated
border-left: size.$edge-medium solid color.$base-gray-light; | ||
color: color.$base-gray-dark; |
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.
We should include styles for our dark theme as well.
<footer> | ||
<cite>Matt Mullenweg</cite> | ||
</footer> |
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.
Did you verify that WordPress includes the footer
element? If not, then we probably can't modify this markup.
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 didn't, I'll double check that.
src/components/comment/comment.scss
Outdated
@include theme.props() { | ||
--theme-color-border-blockquote: #{color.$border-blockquote-light}; | ||
--theme-color-text-blockquote: #{color.$text-blockquote-light}; | ||
} | ||
|
||
@include theme.props(dark) { | ||
--theme-color-border-blockquote: #{color.$border-blockquote-dark}; | ||
--theme-color-text-blockquote: #{color.$text-blockquote-dark}; | ||
} |
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.
Reading this now, I think for the text you might be able to use --theme-color-text-muted
instead of defining new properties and tokens? See src/base/_themes.scss
I could see adding a new border property in the base themes as well, esp. since we're wanting to coordinate that color with comment threads.
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.
Cool! I didn't know we had theme tokens. I updated the color and border color properties so that we're using existing tokens.
} | ||
|
||
.c-comment__content blockquote cite::before { | ||
@include emdash.content; |
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.
Just curious if we could simplify by using an em dash directly instead of needing the \
code?
content: '—';
src/components/comment/comment.scss
Outdated
/// those comments visually with their parent. | ||
.c-comment.is-replying::after, | ||
.c-comment--thread::after { | ||
background-color: var(--theme-color-border-kbd); |
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.
Can we create a border property specifically for this? kbd
refers to the kbd
element, so I'm not sure it's the most intuitive thing to use here. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/kbd
If it happens to be the right color, maybe we just need to rename it wherever it's referenced to something more general.
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'm not sure it's the most intuitive thing to use here
Haha probably not 😆. I added a property specifically for the border color.
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.
Great work, @dromo77! Thanks for weathering the rounds of feedback.
Overview
This PR updates the pull quote and block quotes styles. It uses the Gutenberg block classes to apply styles so that the
blockquote
element remains fairly unopinionated. It also applies those same styles to allblockquote
elements within thec-comment
component.Screenshots
Testing