Improve style for quoted text in RichContent #412

Open
ilja wants to merge 4 commits from ilja/akkoma-fe:improve_visual_style_for_quoted_text into develop
Contributor

Previously quoted text (e.g. in Markdown > Some text) was italic
When two different quotes were made, there was no distinction between the two, making it look like one quote
This is confusing

Now we have a vertical line in front of the quote
When two different pieces of text are quoted, it is now clear because the lines are separated
This vertical line is a typical way of visualising quoted text, so it should be easy to understand what it is

Examples:

  • The italic is how it is before this PR. There are two quotes, but this is unclear from the view.
  • Another is the same post, but with the current PR. It is clear what the two different quotes are (and imo more pretty).
  • I also have an example showing that multiple lines are still possible in one quote. (Don't forget, to have a newline with the Markdown option, you need two spaces at the end of the previous line. This is true when not using quotes, but just as true with using quotes.)
Previously quoted text (e.g. in Markdown `> Some text`) was italic When two different quotes were made, there was no distinction between the two, making it look like one quote This is confusing Now we have a vertical line in front of the quote When two different pieces of text are quoted, it is now clear because the lines are separated This vertical line is a typical way of visualising quoted text, so it should be easy to understand what it is Examples: * The italic is how it is before this PR. There are two quotes, but this is unclear from the view. * Another is the same post, but with the current PR. It is clear what the two different quotes are (and imo more pretty). * I also have an example showing that multiple lines are still possible in one quote. (Don't forget, to have a newline with the Markdown option, you need two spaces at the end of the previous line. This is true when not using quotes, but just as true with using quotes.)
ilja added 1 commit 2024-08-21 16:09:02 +00:00
Improve style for quoted text in RichContent
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
51a51fe6b8
Previously quoted text (e.g. in Markdown `> Some text`) was italic
When two different quotes were made, there was no destinction between the two, making it look like one quote
This is confusing

Now we have a vertical line in front of the quote
When two different pieces of text are quoted, it is now clear because the lines are separated
This vertical line is a typical way of visualising quoted text, so it should be easy to understand what it is
Author
Contributor

One problem I see with this is when reparsing MFM from *key instances... See https://ilja.space/notice/Al8CNka0RS1GrnMnrc

Consider the following as input for MFM:

> Some quoted line
a new line

Akkoma will consider this part of the same quote, *key will only consider the first line as a quote, not the second one.

Before this PR the same thing is true, but bc the quotes are basically just italics, it may be less obvious.

One problem I see with this is when reparsing MFM from *key instances... See https://ilja.space/notice/Al8CNka0RS1GrnMnrc Consider the following as input for MFM: ``` > Some quoted line a new line ``` Akkoma will consider this part of the same quote, *key will only consider the first line as a quote, not the second one. Before this PR the same thing is true, but bc the quotes are basically just italics, it may be less obvious.
Oneric approved these changes 2024-08-25 00:53:20 +00:00
Dismissed
Oneric left a comment
Member

I don’t think the pre-existing MFM inportability is a blocker; apart from italic quotes wree already indented before so this wasn’ŧ really better before.

Note there’s also a pre-existing vertical spacing issue; see: #347. It still exists but isn’t introduced by the changes here. The apparently relevant whitespace classes are beyond my CSS knowledge; if you know more and can fix this properly great, otherwise a quick hack to paper over it though is adjusting margns to margin: 0.2em 0 1.0em 0.2em

I don’t think the pre-existing MFM inportability is a blocker; apart from italic quotes wree already indented before so this wasn’ŧ really better before. Note there’s also a pre-existing vertical spacing issue; see: #347. It still exists but isn’t introduced by the changes here. The apparently relevant whitespace classes are beyond my CSS knowledge; if you know more and can fix this properly great, otherwise a quick hack to paper over it though is adjusting margns to `margin: 0.2em 0 1.0em 0.2em`
@ -5,0 +3,4 @@
margin: 0.2em 0 0.2em 0.2em;
padding: 0 0 0 1em;
border-width: 0 0 0 0.3em;
border-style: solid;
Member

suggestion, not a blocker: i’ve also commonly seen text colour being a bit more muted for quoted sections. We could achieve this with something like this:

color: color-mix(in srgb-linear, var(--text), var(--bg) 50%);

or introduce a new style property for quoted text colour. The former may be undesirable in some themes whose text has a poor contrast by default.

suggestion, not a blocker: i’ve also commonly seen text colour being a bit more muted for quoted sections. We could achieve this with something like this: ```css color: color-mix(in srgb-linear, var(--text), var(--bg) 50%); ``` or introduce a new style property for quoted text colour. The former may be undesirable in some themes whose text has a poor contrast by default.

yeah i think i agree, muted quotations seems to be the norm

yeah i think i agree, muted quotations seems to be the norm
Author
Contributor

Is fixed for making it faint. I did the same way as how making text faint is done in other locations. It's the same as e.g. the timestamp right-top of the status.

When making a theme for akkoma-fe, there's a check for how readable things remain. I'm unsure how that exactly works, but by using the same way of doing this, i'm assuming this is also taken into account for the theming engine.

There's still some weirdness with space, but that was already a problem. I'll see if I can figure out what's going on there.

Here's how it looks now, and i'm taking the opportunity to show how nested quotes are also nicely working ^^

image

And how it is without these changes

image

Is fixed for making it faint. I did the same way as how making text faint is done in other locations. It's the same as e.g. the timestamp right-top of the status. When making a theme for akkoma-fe, there's a check for how readable things remain. I'm unsure how that exactly works, but by using the same way of doing this, i'm assuming this is also taken into account for the theming engine. There's still some weirdness with space, but that was already a problem. I'll see if I can figure out what's going on there. Here's how it looks now, and i'm taking the opportunity to show how nested quotes are also nicely working ^^ ![image](/attachments/9dbc4dbb-1732-43e4-9a06-8d743fa91d49) And how it is without these changes ![image](/attachments/b4a5c7cd-2aab-4652-b783-8f8523c6f1cc)
Author
Contributor

The comment about the space between a paragraph and blockquote is now also fixed 🎉

image

The reason is; p-tags only have margin at the bottom. Blockquotes are block elements on their own, but the margin was different. This gave this problem. See screenshots

Here we check the p-tag. We see there's only a margin at the bottom. That margin is 1em and is what's pushing the next block 1em down.

image

But when we check the blockquote, we see it has margins all around. All are 0.2em, so it only pushes the block below 0.2em down. Between it and the block above, the space is 1em, because there the p-tag has it's 1em margin.

image

Other block elements I can think of who may have similar problems are code, ul and ol-tags. I checked those, and visualy they seem OK. Checking with developer tools (F12 in browseer), it seems they have 1em margin bottom and top, always. We could nitpick wether those should be better to have 0 as last-child, and if they need the margin-top, but if someone wants that, I prefer to have that discussion somewhere else and keep this MR to the blockquotes. If we have that discussion, we should start by checking what elements we allow through the scrubber.

That leaves the problem of the blockqoute looking weird when it comes after a mention, I'll see if I can figure that one out too.

The comment about the space between a paragraph and blockquote is now also fixed 🎉 ![image](/attachments/4ecfae02-4ac7-48dc-a044-ae01e84047b1) The reason is; p-tags only have margin at the bottom. Blockquotes are block elements on their own, but the margin was different. This gave this problem. See screenshots Here we check the p-tag. We see there's only a margin at the bottom. That margin is 1em and is what's pushing the next block 1em down. ![image](/attachments/eea19f76-16d4-4a0c-9f94-c9209046db9b) But when we check the blockquote, we see it has margins all around. All are 0.2em, so it only pushes the block below 0.2em down. Between it and the block above, the space is 1em, because there the p-tag has it's 1em margin. ![image](/attachments/a9afec22-7e15-40d2-8479-c7358598cd65) Other block elements I can think of who may have similar problems are code, ul and ol-tags. I checked those, and visualy they seem OK. Checking with developer tools (F12 in browseer), it seems they have 1em margin bottom and top, always. We could nitpick wether those should be better to have 0 as last-child, and if they need the margin-top, but if someone wants that, I prefer to have that discussion somewhere else and keep this MR to the blockquotes. If we have that discussion, we should start by checking what elements we allow through the scrubber. That leaves the problem of the blockqoute looking weird when it comes after a mention, I'll see if I can figure that one out too.
Author
Contributor

That last problem is also fixed with #347 already. Is there a reason why #347 is still open? Because I think it makes more sense to merge that first. After that I can fix merge conflix on this PR, and then we'll have nice looking quote posts.

That last problem is also fixed with https://akkoma.dev/AkkomaGang/akkoma-fe/pulls/347 already. Is there a reason why https://akkoma.dev/AkkomaGang/akkoma-fe/pulls/347 is still open? Because I think it makes more sense to merge that first. After that I can fix merge conflix on this PR, and then we'll have nice looking quote posts.
Author
Contributor

hm 🤔 I do wonder why that

  .text {
    & > * {
      white-space: pre-wrap;
    }

is for. When I just remove it, it also works. AND when I remove it, codeblocks work better imo. The change in #347 makes the lines "wrap". By removing it, we get a scrollbar.

This is with removing the line

image

This is with the fix from #347

image

I'll make the commit as the first example (where we have the scroll bar) and leave it up to others to decide what they like best.

hm 🤔 I do wonder why that ``` .text { & > * { white-space: pre-wrap; } ``` is for. When I just remove it, it also works. AND when I remove it, codeblocks work better imo. The change in #347 makes the lines "wrap". By removing it, we get a scrollbar. This is with removing the line ![image](/attachments/8c5d13a0-2ec3-4699-8311-3d630545c9db) This is with the fix from #347 ![image](/attachments/a97b1d22-1811-4343-b13d-2d533896cea8) I'll make the commit as the first example (where we have the scroll bar) and leave it up to others to decide what they like best.
Author
Contributor

Pushed, I think it's good now. Here's a last screenshot showing blockquotes and different spacings.

image

Pushed, I think it's good now. Here's a last screenshot showing blockquotes and different spacings. ![image](/attachments/258eaefd-f2c5-465e-b874-347231e445c0)
113 KiB
Author
Contributor

Seems it was an akkoma regression when trying to get mfm to work. Before that, the code I now removed was also just not there. I'm unsure why it was added. $[spin something] seems to still work for example.

@nbsp Do you happen to remember what this white-space: pre-wrap was for (just to be sure we're not breaking other things)? In src/components/status_body/status_body.scss

  .text {
    & > * {
      white-space: pre-wrap;
    }

commit: 931ed3d3c7 (diff-4200029adfe97f22b2316110f9e4adafca58f990)

pr: #29

Seems it was an akkoma regression when trying to get mfm to work. Before that, the code I now removed was also just not there. I'm unsure why it was added. `$[spin something]` seems to still work for example. @nbsp Do you happen to remember what this `white-space: pre-wrap` was for (just to be sure we're not breaking other things)? In src/components/status_body/status_body.scss ``` .text { & > * { white-space: pre-wrap; } ``` commit: https://akkoma.dev/AkkomaGang/akkoma-fe/commit/931ed3d3c77293e1d6087c039e2a613962b00781#diff-4200029adfe97f22b2316110f9e4adafca58f990 pr: https://akkoma.dev/AkkomaGang/akkoma-fe/pulls/29
ilja added 1 commit 2024-10-12 13:45:23 +00:00
Make text faint for blockquotes
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
a9367d444a
In code review it was decided that faint text for blockquotes should be used.
I copied how it was done in other places to faint text.

When making a theme for *oma-fe, there's a check for how readable things remain.
I'm unsure how that exactly works, but timestamps for a status is also faint,
so by using the same way of doing this, this should also be taken into account
for the theming engine.
ilja added 1 commit 2024-10-12 15:34:50 +00:00
Fix spacing between paragraph and blockquote
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
fa058ca093
Blockquotes are blocks, so not wrapped in an extra p-tag.

In statusses this gave an unfortunate result that the margins were different.
A p-tag has a bottom margin of 1em. Blockquotes had 0.2em top and bottom.

So under a paragraph there was 1em space, but under the blockquote, there was only 0.2em space.

The last p-tag has 0 margin at the bottom.

This commit basically does the same thing for blockquotes now, making it more consistent.

One difference is that the blockquote has a left margin of 0.2em because a little "jump"
in makes it look a bit better imo.
ilja added 1 commit 2024-10-12 16:29:14 +00:00
Remove pre-wrap from status_body
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
01164fc520
For some strange reason, after a mention a quote would be double as high as it should.

Removing this "pre-wrap" seems to fix this. I'm not sure what it was exactly for, but I don't see anything break.

The code blocks now don't wrap any more, but show a scroll bar, which imo is better for a code block.
Oneric approved these changes 2024-10-13 22:47:06 +00:00
Oneric left a comment
Member

Seems to work fine in testing; thanks!

Nice find re --faint.
I’m not really qualified to judge the whitespace: pre-wrap stuff, but the explanation sounds convincing and i didn’t encounter any obvious issues so far. With the scroll bar disappearing by default in Firefox at least, the presence of overflowed content might not be obvious if the overflow occurs within whitespace or on a character boundary, but this is a matter for a different PR imho (e.g. distinguishing code blocks with a different background colour, which might also necessitate a different text color for readability unless we already have something fitting?)

Seems to work fine in testing; thanks! Nice find re `--faint`. I’m not really qualified to judge the `whitespace: pre-wrap` stuff, but the explanation sounds convincing and i didn’t encounter any obvious issues so far. With the scroll bar disappearing by default in Firefox at least, the presence of overflowed content might not be obvious if the overflow occurs within whitespace or on a character boundary, but this is a matter for a different PR imho *(e.g. distinguishing code blocks with a different background colour, which might also necessitate a different text color for readability unless we already have something fitting?)*
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u improve_visual_style_for_quoted_text:ilja-improve_visual_style_for_quoted_text
git checkout ilja-improve_visual_style_for_quoted_text
Sign in to join this conversation.
No description provided.