Skip to content
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.

Sign up for GitHub

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

Jump to bottom

content: Support Zulip content outside messages (even outside per-account contexts) #488

Open
chrisbobbe opened this issue Jan 19, 2024 · 4 comments
Open

content: Support Zulip content outside messages (even outside per-account contexts) #488

chrisbobbe opened this issue Jan 19, 2024 · 4 comments
Labels
a-content Parsing and rendering Zulip HTML content, notably message contents
Milestone
Launch

Comments

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Jan 19, 2024 •
edited

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:

image

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 uses RealmContentNetworkImage, which requires a PerAccountStoreWidget 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:

BlockContentList(nodes: parseContent(widget.serverSettings.realmDescription).nodes)

But the links don't work; an error prints to the console when I tap one of them.
@chrisbobbe chrisbobbe added the a-content Parsing and rendering Zulip HTML content, notably message contents label Jan 19, 2024
@chrisbobbe
Copy link
Collaborator Author

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.

@chrisbobbe

This comment was marked as resolved.

Sign in to view
@gnprice

This comment was marked as resolved.

Sign in to view
@chrisbobbe chrisbobbe mentioned this issue Feb 12, 2024
Merged
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Feb 24, 2024
@chrisbobbe
content test: Support localizable UI text in smoke tests
e3e52db 
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
@chrisbobbe
content test: Support localizable UI text in smoke tests
edaf530 
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
@chrisbobbe @gnprice
content test: Support localizable UI text in smoke tests
4663118 
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.
@alya
Copy link
Collaborator

alya commented Apr 24, 2024 •
edited

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.

@gnprice gnprice added this to the Launch milestone May 9, 2024
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 13, 2024
@chrisbobbe
content test: In prepareContentBare, don't include Scaffold by default
b1c6443 
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
@chrisbobbe
content tests [nfc]: Deduplicate ad-hoc content-preparing code
223d633 
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
@chrisbobbe
content tests [nfc]: Deduplicate ad-hoc content-preparing code
cc8c9fe 
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 chrisbobbe mentioned this issue May 13, 2024
Merged
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue May 13, 2024
@chrisbobbe
content tests [nfc]: Deduplicate ad-hoc content-preparing code
7050c3c 
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
@chrisbobbe
content test [nfc]: Deduplicate ad-hoc content-preparing code
e151e61 
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
@chrisbobbe
content test [nfc]: Deduplicate ad hoc content-preparing code
b011d2f 
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
Assignees
No one assigned
Labels
a-content Parsing and rendering Zulip HTML content, notably message contents
Projects
Flutter app
Status: No status
Milestone
Launch
Development

No branches or pull requests
3 participants
@gnprice @alya @chrisbobbe