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.

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

WIP: Feature: Add Spoiler support #1061

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Ezio-Sarthak
Copy link
Member

@Ezio-Sarthak Ezio-Sarthak commented Jul 2, 2021

@preetmishra Thanks for your primary work on this in a local branch, that helped me structuring this PR with separate commits and additional tests. This PR adds a basic support for spoilers in ZT.

Fixes #688.

Potential caveats

  • The links in spoilers are currently revealed in message info view, which isn't expected.

Commit flow:

  • boxes/themes: Add support for spoiler header markup in MessageBox.
  • boxes/views: Extract spoiler content in a variable in MessageBox.
  • core/views: Add SpoilerView class and corresponding show_* function.
  • boxes/core/views: Pass spoiler data to MsgInfoView popup helpers.
  • buttons: Add SpoilerButton class showing spoiler header.
  • views: Show SpoilerButton in message info view if present.

Screenshots/GIFs

spoiler_view

@Ezio-Sarthak Ezio-Sarthak added area: message rendering enhancement New feature or request feedback wanted PR needs review PR requires feedback to proceed labels Jul 2, 2021
preetmishra and others added 5 commits July 3, 2021 17:25
This commit:
 * adds new style (LIGHTGREENBOLD, LIGHTGREENBOLD24) for spoiler header
   in `themes.py`.
 * adds markup entry in `soup2markup` for spoilers.

Tests added.

Co-authored-by: Sarthak Garg <grgsrthk.dev20@gmail.com>
This commit handles content metadata in `soup2markup` for spoilers.

Test amended.

Co-authored-by: Preet Mishra <ipreetmishra@gmail.com>
This commit adds popup to show hidden spoiler content.

Tests added.

Co-authored-by: Preet Mishra <ipreetmishra@gmail.com>
Tests amended.

Co-authored-by: Preet Mishra <ipreetmishra@gmail.com>
This commit adds a button that would allow user to view the spoiler
content, i.e., onclick of button is SpoilerView popup.

Tests added.

Co-authored-by: Preet Mishra <ipreetmishra@gmail.com>
This commit wraps-up the PR by appending the SpoilerButton to message
info view if present.

Fixes zulip#688.

Co-authored-by: Preet Mishra <ipreetmishra@gmail.com>
@neiljp
Copy link
Collaborator

neiljp commented Jul 4, 2021

@Ezio-Sarthak I just gave this a try - it looks good, and is a definite improvement over just showing the content like now!
Thoughts:

  • It's quite easy to lose which spoiler I'm looking at, ie. basically the context. So, including at least the spoiler title, or maybe more details of the message, seems important?
  • I also felt confused when opening the spoiler popup and what to do "next" - I think most of our secondary popups have assigned keys, so exiting can return to the message info, for example.
  • The design suffers from the table limitations (I've not checked if you reuse the code), and long spoiler names break the table
  • Links in spoilers "leak out" via footlinks? (edit: you mentioned that in the description 👍)

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jul 4, 2021
@zulipbot
Copy link
Member

Heads up @Ezio-Sarthak, 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/main branch and resolve your pull request's merge conflicts accordingly.

@neiljp
Copy link
Collaborator

neiljp commented Feb 4, 2024

As per the issue, this set of features was discussed in the topic starting at https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Spoilers!.20.23T688.20.23T1061.20.231173/near/904352.

@rsashank
Copy link
Member

rsashank commented Feb 4, 2024

I'll be looking into this, will submit a WIP PR soon.

@neiljp neiljp added missing feature: user A missing feature for all users, present in another Zulip client and removed enhancement New feature or request labels Apr 22, 2024
@rsashank rsashank mentioned this pull request Jul 3, 2024
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: message rendering feedback wanted has conflicts missing feature: user A missing feature for all users, present in another Zulip client PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback PR completion candidate PR with reviews that may unblock merging version parity: 3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for spoilers
5 participants