zulip
markdown: Add support for admonitions
This adds support for admonitions, as documented here:
https://squidfunk.github.io/mkdocs-material/extensions/admonition/
See further premliminary discussion here: https://chat.zulip.org/#narrow/stream/6-frontend/topic/message.20status.20codes
The idea is that this gives users two types of syntax for producing colored blocks within messages. The primary use case for this is colorizing status messages from bots (think monitoring messages, or a build pass/fail message).
The standard admonition sytax is a triple bang, admonition class, and optional title. Content of the block is indented four spaces:
!!! success "Build succeeded!"
The build was a succeess.
I extended the admonition parser to additionally support an unindented fenced syntax:
!!! success "Build succeeded!"
The build was a success.
!!!
Furthermore, I modified the admonition renderer so that it renders a
<blockquote> rather than the default of <p class="admonition ...">. This
was done so that clients which are otherwise ignorant of the admonition
classes will still render the block reasonably.
I supported the following classes with CSS:
- red - danger, error, critical
- yellow - warning, caution
- green - success
- blue - info, primary
- gray - neutral, secondary
This scheme, and the colors themselves, as you might guess, was lifted directly from the standard Bootstrap palette for colored elements.
Testing Plan:
I've tested a variety of syntaxes locally, and added a couple of tests to the Bugdown suite. Current shortcomings:
- Because this is implemented as a block parser, blank spaces inside the admonition block cause the admonition block to terminate.
- I couldn't figure out how to use things like code blocks inside the admonition.
GIFs or Screenshots:
The screenshots look great, and the CSS looks clean! We'll likely want to iterate a bit on the backend implementation.
Have you seen https://zulip.readthedocs.io/en/latest/subsystems/markdown.html? It's not absolutely critical for this to be handled by the frontend markdown processor, but it has useful background.
Also, can you update templates/zerver/help/format-your-message-using-markdown.md to document this feature? I'm not sure whether it is important enough to go in templates/zerver/app/markdown_help.html.
I have read through that doc; I didn't touch the frontend renderer just yet because this was enough for an MVP, and I figured it'd get the ball rolling on discussion and improvement before I did too much work in the wrong direction.
markdown.contains_backend_only_syntax would be a short path to getting this shipped, but ideally the frontend could support it, too, though as noted in the original discussion thread, this isn't terribly likely to be heavily used by humans directly.
markdown.contains_backend_only_syntax would be a short path to getting this shipped, but ideally the frontend could support it, too, though as noted in the original discussion thread, this isn't terribly likely to be heavily used by humans directly.
Yeah, exactly, I was thinking it'd be worth moving the tests to the shared test suite with just contains_backend_only_syntax and the flag for the syntax being different would be a cleaner intermediate state. And then we can open a follow-up issue for adding local echo support for it.
One thing we should do before pushing this out is to make sure we have a test that prevents some malicious user trying to send a message like !!! <script src="send_all_your_info_to_me>. And I think we even want to prevent more innocuous hacks like !!! condensed.
The way I would make these tests pass is to have the code only allow the known variations (warning, error, etc.) during the parse.
I added the prefixing specifically to avoid the innocent namespace collisions (!!! condensed would become <blockquote class="admonition admonition-condensed">), but an explicit whitelist is probably a wise route. I've updated the implementation accordingly. I added a few tests as well (I've been amending the commit and force-pushing to keep the eventual history clean; is it more preferred to just accumulate fixit commits on the branch, and then squash during merge?)
One argument against whitelisting might be that it would permit future use of admonitions to "just work" by tweaking delivered CSS. For example, if down the road we were to want to add a "large" class which would increase the font size for that admonition, any admonitions previously sent as !!! info large or whatnot would Just Work, whereas large would get stripped under the current whitelisting scheme. I don't feel strongly about it either way though.
The blue text on the night-mode background in that image is actively painful to read.
It's better on what my system renders as the night-mode background, which is much darker – but it could still use some tweaking.
Although I guess this was sort of my idea given the use case, I’m always concerned that adding extensions outside of CommonMark will make it harder to get our Markdown parsers to a state we’ll be happy with. There are fundamental reasons why both Python-Markdown and Marked are unsuitable starting points and they’ll both need to be replaced in the long term. So we should be really confident that this use case is important.
I’m not a big fan of the rendering as a colored vertical bar. We already use colored vertical bars to indicate (1) stream messages and (2) unread messages, and that’s already one too many things. Maybe a 1px border would work better?
I’d prefer to avoid the blockquote hack, because it will make these annoying to restyle going forward forever. Existing clients will be updated quickly enough.
The tests should go in zerver/tests/fixtures/markdown_test_cases.json with "backend_only_rendering": true.
The vertical bar is admittedly my bias as a user coming from Slack; it works well there, and it's comfortable and familiar. I suspect most people that are looking for this kind of feature are coming from Slack and are looking for something similar, so there's something to be said for familiarity. I'm open to exploring other ideas, though. FWIW, the green side border for unread marking is consistently confusing to me, in large part because it's only briefly visible as I just start parsing the message, then disappears.
Easy enough to move back to a div from the blockquote. The idea was generally to be able to ensure consistent styling and vertical alignment of text as you scrolled down the stream, but that's possible to do with a plain div, too.
I'll get the tests moved.
Here's one idea: status dots. Don't think I love it.
1px borders. I don't think I like this at all.
By comparison, here's what the similar feature looks like in Slack:
The left borders (even if we render them with divs and not blockquotes) have the advantage of being visually aligned and consistent with blockquotes
A slightly thinner border distinguishes them from blockquotes, but preserves horizontal alignment
On the question of how to style it: FWIW I've never loved the left-borders design as it appears in the Slack version of this feature. One thing is that it tends to put the information exclusively at the far left of the message, and it doesn't feel like something that's meant to flavor the whole message.
The thin border all around looks like one good direction to me.
Independent of what we do for where to put a color, we should also include a design element that's not based on color, so that we don't find ourselves shipping something that isn't accessible. Adding a different emoji for each type could be one good solution. Perhaps :warning: U+26A0 WARNING SIGN for "warning"/"caution"? And... :x: U+274C CROSS MARK for "error"? (Many other choices are possible too.)
Another possible way to present the color (in addition to Slack's heavy left border, or a thin all-around border): a style that I see fairly often in a variety of documentation on the Web, and I think works pretty well, involves a mildly colored background. E.g. a red background for a stern warning, and a yellow or orange background for a milder caution.
Here's a set of examples in a Google style guide for developer documentation:
Other variations combine this with a thin border in a more saturated color of similar hue, or with a heavy left border.
Colored backgrounds can work. There's already a colored background in play for messages where a user is mentioned (a dark red, iirc, which feels a little error-y to me). Due to the fact that the admonition is a block in the message, it'd be possible to have a green success block stacked on top of a red highlight block, which could be kind of odd.
I don't particularly like the emoji addition just because it "forces" more meaning into the element outside of the message content. My thought is that this kind of thing is really just a tool for bots/integrations to flavor their messages with, and emoji pertinent to the message should probably be delivered as a part of the message content. For example, I could include a cross mark as a part of the admonition title, if I wanted to include it.
For cases where color alone is insufficient, my inclination would be to offer an (app-wide) colorblind mode as an option which augments elements that rely on colorblind-sensitive colors with alternative presentations via CSS, to keep the treatment minimalistic for the common case. An ARIA label could help with accessibility concerns, as well.
I don’t have a problem with forcing meaning into the element—that helps us reserve the right to overhaul our design language later in a way that’s compatible with the intended semantics and not necessarily with a specific presentation.
That’s also a good reason to avoid overly “canonicalizing” the types in the rendered HTML.
I’ve never heard of a “primary” or “secondary” admonition type. Ideally we’d keep a similar list of types to other admonition implementations (e.g. reST, mkdocs-material, Asciidoc, which are admittedly somewhat different already).
Heads up @cheald, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.
