-
Notifications
You must be signed in to change notification settings - Fork 21
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
Updating QuoteBlock #39
base: main
Are you sure you want to change the base?
Conversation
I made a few design changes and an additional change to make the ux better since quotes are often long and forcing users to put them in a single-line CharBlock is a poor experience. |
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.
Thanks! The end result looks good, but I'm not sure about some of the decisions here.
{{ self }} | ||
</div> | ||
{% endverbatim %} | ||
<div class="rich-text mb-10 md:mb-20 last:mb-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.
This doesn't use rich text so I think we should remove it
<div class="rich-text mb-10 md:mb-20 last:mb-0"> | |
<div class="mb-10 md:mb-20 last:mb-0"> |
@@ -1,5 +1,19 @@ | |||
{% verbatim %} |
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.
Could you reinstate the verbatim
wrapper please? Otherwise the tags inside will be interpreted as part of the project template rendering process.
{% if large_spacing %} | ||
mb-10 md:mb-20 | ||
{% else %} | ||
mb-8 md:mb-10 | ||
{% endif %} |
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 where large_spacing
would come from for this block – is this intentional?
</div> | ||
{% endverbatim %} | ||
<div class="rich-text mb-10 md:mb-20 last:mb-0"> | ||
<h2 class="font-serif4 text-3xl md:text-5xl font-semibold text-center |
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.
Do we intentionally want the quote and attribution to be headings (in terms of semantics)? We can keep the large font with text-3xl md:text-5xl
, but I don't know if it should be a heading.
Also, I'm not sure about text-center
. I've heard opinions that it's less readable. Though it's probably not that big of an issue for brief text like quotes with a large font.
"> | ||
"{{ value.quote }}"</h2> | ||
{% if value.attribution %} | ||
<h2 class="font-serif4 text-base md:text-xl font-medium text-center |
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.
If we use a heading, it probably makes more sense for this to be a lower-level heading i.e. h3 so it's under the quote's heading instead of being adjacent.
This PR addresses the QuoteBlock component of #38
I have removed the link from QuoteBlock because adding links to quote blocks is not a common feature for most publications and leaving it in would be confusing to most folks familiar with online publishing.
I have added in a basic HTML template but will leave this as a draft to give me some time to figure out our Tailwind classes and determine if there is better basic design option for this block.