-
Notifications
You must be signed in to change notification settings - Fork 270
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
content: Support Zulip content outside messages (even outside per-account contexts) #488
Labels
a-content
Parsing and rendering Zulip HTML content, notably message contents
Milestone
Comments
So, we need the content widgets to work even when their message- and account-data dependencies are absent. Ideally, we would have a way to enforce that such strict dependencies aren't reintroduced. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
chrisbobbe
added a commit
to chrisbobbe/zulip-flutter
that referenced
this issue
Feb 24, 2024
When we support spoilers, soon, we'll want to translate the default header text ("Spoiler") into the user's language. It seems safe to assume that Zulip content will always have `ZulipLocalizations.of` support wherever it appears in the widget tree. The language will be a client setting with a default value. There are certain other assumptions that `prepareContentBare` will probably want to avoid making, though, including: - that the content is rendered in a particular message - that the content is rendered in a message at all (zulip#488) - that the content is rendered in a per-account context (zulip#488) But we'll think more about that later, when we start using testContentSmoke in more places.
chrisbobbe
added a commit
to chrisbobbe/zulip-flutter
that referenced
this issue
Feb 26, 2024
When we support spoilers, soon, we'll want to translate the default header text ("Spoiler") into the user's language. It seems safe to assume that Zulip content will always have `ZulipLocalizations.of` support wherever it appears in the widget tree. The language will be a client setting with a default value. There are certain other assumptions that `prepareContentBare` will probably want to avoid making, though, including: - that the content is rendered in a particular message - that the content is rendered in a message at all (zulip#488) - that the content is rendered in a per-account context (zulip#488) But we'll think more about that later, when we start using testContentSmoke in more places.
gnprice
pushed a commit
to chrisbobbe/zulip-flutter
that referenced
this issue
Feb 26, 2024
When we support spoilers, soon, we'll want to translate the default header text ("Spoiler") into the user's language. It seems safe to assume that Zulip content will always have `ZulipLocalizations.of` support wherever it appears in the widget tree. The language will be a client setting with a default value. There are certain other assumptions that `prepareContentBare` will probably want to avoid making, though, including: - that the content is rendered in a particular message - that the content is rendered in a message at all (zulip#488) - that the content is rendered in a per-account context (zulip#488) But we'll think more about that later, when we start using testContentSmoke in more places.
However we end up presenting stream descriptions, we'll probably want to show descriptions for views the same way as well. Web app issue: zulip/zulip#29769. |
chrisbobbe
added a commit
to chrisbobbe/zulip-flutter
that referenced
this issue
May 13, 2024
None of our content widgets currently depend on a Scaffold. It's not necessarily bad if they do -- but if it ever happens, it'll be helpful to be explicit about it, and using this param would be a way to get that explicitness. (I assume the explicitness would be helpful when we do zulip#488, "content: Support Zulip content outside messages (even outside per-account contexts)".)
chrisbobbe
added a commit
to chrisbobbe/zulip-flutter
that referenced
this issue
May 13, 2024
It should now be easy to drop in natural dependencies of our content widgets, such as a ThemeExtension for custom light- and dark-theme custom colors, for all the tests of our Zulip content widgets. To do that, we can just add unconditional code to this function. It should also help us keep being explicit about some potentially problematic dependencies, which will be handy when we work on zulip#488 ("content: Support Zulip content outside messages (even outside per-account contexts)"). After this change, some ad-hoc `tester.pumpWidget` calls do remain in this file; see tests of RealmContentNetworkImage and AvatarImage. But those aren't "Zulip content widgets" in the same sense as most of the widgets in lib/widgets/content.dart, which are used exclusively to render parsed Zulip Markdown. (There isn't even any Zulip Markdown that would produce an AvatarImage.) So, leave them be. Perhaps these widgets belong in some other file.
chrisbobbe
added a commit
to chrisbobbe/zulip-flutter
that referenced
this issue
May 13, 2024
It should now be easy to drop in natural dependencies of our content widgets, such as a ThemeExtension for custom light- and dark-theme custom colors, for all the tests of our Zulip content widgets. To do that, we can just add unconditional code to this function. It should also help us keep being explicit about some potentially problematic dependencies, which will be handy when we work on zulip#488 ("content: Support Zulip content outside messages (even outside per-account contexts)"). After this change, some ad-hoc `tester.pumpWidget` calls do remain in this file; see tests of RealmContentNetworkImage and AvatarImage. But those aren't "Zulip content widgets" in the same sense as most of the widgets in lib/widgets/content.dart, which are used exclusively to render parsed Zulip Markdown. (There isn't even any Zulip Markdown that would produce an AvatarImage.) So, leave them be. Perhaps these widgets belong in some other file. Related: zulip#95 Related: zulip#488
chrisbobbe
added a commit
to chrisbobbe/zulip-flutter
that referenced
this issue
May 13, 2024
It should now be easy to drop in natural dependencies of our content widgets, such as a ThemeExtension for custom light- and dark-theme colors, for all the tests of our Zulip content widgets. To do that, we can just add unconditional code to this function. It should also help us keep being explicit about some potentially problematic dependencies, which will be handy when we work on zulip#488 ("content: Support Zulip content outside messages (even outside per-account contexts)"). After this change, some ad-hoc `tester.pumpWidget` calls do remain in this file; see tests of RealmContentNetworkImage and AvatarImage. But those aren't "Zulip content widgets" in the same sense as most of the widgets in lib/widgets/content.dart, which are used exclusively to render parsed Zulip Markdown. (There isn't even any Zulip Markdown that would produce an AvatarImage.) So, leave them be. Perhaps these widgets belong in some other file. Related: zulip#95 Related: zulip#488
chrisbobbe
added a commit
to chrisbobbe/zulip-flutter
that referenced
this issue
May 13, 2024
It should now be easy to drop in natural dependencies of our content widgets, such as a ThemeExtension for custom light- and dark-theme colors, for all the tests of our Zulip content widgets. To do that, we can just add unconditional code to this function. It should also help us keep being explicit about some potentially problematic dependencies, which will be handy when we work on zulip#488 ("content: Support Zulip content outside messages (even outside per-account contexts)"). After this change, some ad-hoc `tester.pumpWidget` calls do remain in this file; see tests of RealmContentNetworkImage and AvatarImage. But those aren't "Zulip content widgets" in the same sense as most of the widgets in lib/widgets/content.dart, which are used exclusively to render parsed Zulip Markdown. (There isn't even any Zulip Markdown that would produce an AvatarImage.) So, leave them be. Perhaps these widgets belong in some other file. Related: zulip#95 Related: zulip#488
chrisbobbe
added a commit
to chrisbobbe/zulip-flutter
that referenced
this issue
May 13, 2024
It should now be easy to drop in natural dependencies of our content widgets, such as a ThemeExtension for custom light- and dark-theme colors, for all the tests of our Zulip content widgets. To do that, we can just add unconditional code to this function. It should also help us keep being explicit about some potentially problematic dependencies, which will be handy when we work on zulip#488 ("content: Support Zulip content outside messages (even outside per-account contexts)"). After this change, some ad-hoc `tester.pumpWidget` calls do remain in this file; see tests of RealmContentNetworkImage and AvatarImage. But those aren't "Zulip content widgets" in the same sense as most of the widgets in lib/widgets/content.dart, which are used exclusively to render parsed Zulip Markdown. (There isn't even any Zulip Markdown that would produce an AvatarImage.) So, leave them be. Perhaps these widgets belong in some other file. Related: zulip#95 Related: zulip#488
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The web app supports Zulip Markdown rendering in contexts other than just message content:
See for example
realm_description
from the server settings on CZO, shown on the login page:And see
rendered_description
on stream objects in the/register
response, shown in stream settings. This screenshot shows links (like[text](link)
) being rendered in some stream descriptions:This kind of thing would be easy to support in our app, except that some of the widgets in
lib/widgets/content.dart
have grown dependencies on data that won't be available in these other contexts. For example,MessageImage
usesRealmContentNetworkImage
, which requires aPerAccountStoreWidget
ancestor. It also links to the lightbox, which is also per-account and additionally pulls data about the message's sender.Currently, the following single line of code (atop #281) actually succeeds in visually reproducing CZO's realm description:
But the links don't work; an error prints to the console when I tap one of them.
The text was updated successfully, but these errors were encountered: