zulip-terminal
zulip-terminal copied to clipboard
Render Spoilers in ZT
What does this PR do, and why?
Add support for rendering spoilers in messages:
- Add styles for the spoiler header in themes.
- Add spoiler processing to soup2markup.
- Trim spoiler content to 10 characters, appending ... to ensure the table doesn't break.
- Add SpoilerButton class, append spoiler buttons to the message info view.
- Add SpoilerView class to view spoiler content.
EscandEnterreturns to the message info popup;ireturns to the message view.
External discussion & connections
- [x] Discussed in #zulip-terminal in
Spoilers! #T688 #T1061 #T1173 #T1529 - [x] Fully fixes #688
- [ ] Partially fixes issue #
- [x] Builds upon previous unmerged work in PR #1061
- [ ] Is a follow-up to work in PR #
- [ ] Requires merge of PR #
- [ ] Merge will enable work on #
How did you test this?
- [x] Manually - Behavioral changes
- [x] Manually - Visual changes
- [x] Adapting existing automated tests
- [x] Adding automated tests for new behavior (or missing tests)
- [ ] Existing automated tests should already cover this (only a refactor of tested code)
Self-review checklist for each commit
- [x] It is a minimal coherent idea
- [x] It has a commit summary following the documented style (title & body)
- [x] It has a commit summary describing the motivation and reasoning for the change
- [x] It individually passes linting and tests
- [x] It contains test additions for any new behavior
- [x] It flows clearly from a previous branch commit, and/or prepares for the next commit
Visual changes
| Rendered Message | Message Information Popup | Spoiler View |
|---|---|---|
Still working on this; I've created a draft PR if anyone wants to give feedback on it so far. :)
I tested this. It looks pretty cool. I really like the green "Spoiler:" text in zt_dark and zt_blue. Looks fine in gruvbox_dark. I tried testing the light themes, but I gave up when my eyes started hurting. I see that you've chosen to use one of the existing colors for all the themes, did you try out any other colors?
When quoting spoilers, the lines are offset (as expected).
Just mentioning this because I didn't notice it elsewhere in CZO or in the previous PR's comments.
And it gets more offset with more levels of depth, but that's probably quite a rare use case.
The quoted spoilers are also shown in the msg info view, so that's great.
Do we want to consider adding any formatting to the Spoiler View, to separate the header and the content? It looks quite fine now, no doubt, I'm only thinking about aesthetics.
But the "up/down scrolls" in the header felt a bit out of place to me. I appreciate sticking to the convention. But, could we update to include that only when not all of the spoiler is visible, and not show it otherwise?
Multiple spoilers, message links, opening/closing work fine. Handles empty spoilers just like the webapp does, which is displaying an empty message :thinking:. This is good for the PR. But I'm surprised. Maybe, as a followup, we can check for empty spoilers/quotes/links/code blocks/etc before sending messages. Not sure if there are discussions elsewhere regarding this.
Will review the code soon.
Thanks @rsashank for working on this. I tried running this on my local and it worked fine. Apart from the suggestions already given, I don't have much to add for the current iteration of the PR