Skip to content
This repository has been archived by the owner on Dec 2, 2020. It is now read-only.

Try adding blockquote styles #513

Merged
merged 7 commits into from
Oct 19, 2020
Merged

Try adding blockquote styles #513

merged 7 commits into from
Oct 19, 2020

Conversation

kjellr
Copy link
Collaborator

@kjellr kjellr commented Oct 15, 2020

Alternate to #184. I spent a little time giving this a try too, and got it pretty close.

Screen Shot 2020-10-15 at 4 04 10 PM

Should work well with left/center/right-aligned quotes. The only bug I know of at the moment is that the extra margin I add on small screen sizes to house the blockquotes is eaten up by higher-specificity global margins on the front end:

Screen Shot 2020-10-15 at 4 07 00 PM

I won't be able to work on solving that until tomorrow morning, so if someone else would like to pick this up feel free. If we get #184 cleaned up first, that's fine too. Thanks!

@kjellr kjellr added the [Type] Enhancement New feature or request label Oct 15, 2020
@kjellr kjellr self-assigned this Oct 15, 2020
@ryelle
Copy link
Collaborator

ryelle commented Oct 15, 2020

I've iterated on this a bit to fix up the small screen quotes and some issues in the editor. The only question I have left is whether the de-bolding of strong should also affect the cite content — I've talked this through with @melchoyce and toggling bold on in the quote correctly unbolds the content, but if you bold something in the citation it does nothing right now.

Some screenshots from Mac/Firefox:

Front end, desktop

Screen Shot 2020-10-15 at 19 00 15-fullpage

Editor, desktop

Screen Shot 2020-10-15 at 19 02 01-fullpage

Front-end, mobile

Screen Shot 2020-10-15 at 19 11 51-fullpage

@ryelle
Copy link
Collaborator

ryelle commented Oct 15, 2020

@melchoyce mentioned the citations should not be italics, so I've pushed a fix for that; and I also noticed that things were out of alignment in the editor on a wide screen (wider than my screenshot above), so I've fixed that too.

Copy link
Contributor

@melchoyce melchoyce left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@carolinan
Copy link
Contributor

This is a comparison between this PR (right) and #453

quote

And I think they both have different issues, with the alignments, weight, and the citation.

@kjellr
Copy link
Collaborator Author

kjellr commented Oct 16, 2020

Thank you for the improvements, @ryelle!

I think they both have different issues, with the alignments, weight, and the citation.

Regarding the alignments, this PR intentionally leaves off the quotation mark accent on the center version, and right-aligns it when the quote is right aligned. I prefer that to the treatment in #453.

This PR uses a 600 font weight for the text instead of 700. The theme uses 600 for bold headings and the pagination titles and that's what I pulled that from, but I see that the pullquote block and widget font titles use a 700 weight. We may want to align those, but I'll leave it to @melchoyce to make the final call on that.

It looks like #453 adds a hyphen + a non-breaking space before the citation (If we go with that, typography rulebooks typically suggest it should be an em-dash instead of a hypen). In general though, I personally prefer letting users enter that themselves.

@carolinan
Copy link
Contributor

-Then let's use this one.

I can't take preference in consideration if it is not documented why decisions were made.

@carolinan carolinan mentioned this pull request Oct 16, 2020
4 tasks
@melchoyce
Copy link
Contributor

This PR more accurately reflects the design I intended.

This PR uses a 600 font weight for the text instead of 700. The theme uses 600 for bold headings and the pagination titles and that's what I pulled that from, but I see that the pullquote block and widget font titles use a 700 weight. We may want to align those, but I'll leave it to @melchoyce to make the final call on that.

Let's go with 700 👍

It looks like #453 adds a hyphen + a non-breaking space before the citation (If we go with that, typography rulebooks typically suggest it should be an em-dash instead of a hypen). In general though, I personally prefer letting users enter that themselves.

Yeah, @ryelle asked me about this too, and I said I'd rather leave it up to people to add it if they want it.

@kjellr
Copy link
Collaborator Author

kjellr commented Oct 16, 2020

Let's go with 700

Updated!

This was referenced Oct 16, 2020
@carolinan
Copy link
Contributor

The conflict needs to be resolved, but I am not sure what else this PR is waiting for?

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

Code looks good to me, and since design has already approved it I believe this is OK to merge 👍

@aristath aristath merged commit ac60bec into trunk Oct 19, 2020
@aristath
Copy link
Member

Resolved merge conflicts and merged

@aristath aristath deleted the try/blockquote-styles branch October 19, 2020 08:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
[Type] Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants