Initial support for markdown spoiler tags #867
Conversation
|
Hey I thought I had already commented on this PR, but now I can't find my comment. Fortunately I saved it. Here's what I meant to say. Wow, nice work!! This already looks great! Unfortunately, either I'm doing something wrong, or it doesn't work quite right on Android. Here's what I see on the post you linked. Some comments are empty, others don't respect the spoiler, etc. Can you try in your Android emulator just to see if I'm crazy?
|
|
Hmm, that's really interesting. I'll do a test with an Android emulator when I get the chance and see what the issue here is! |
i have an android phone(samsung galaxy m21s), if you want me test it on hardware i'm happy to help :b |
…/markdown-spoiler
@micahmo I think I figured out the issue - could you try again and see if it works now?
@Vilianexe Thanks for the interest! Do you happen to have some prior background in development or know how to set up your environment for development? If you're curious on how to set up your computer for development or would like to learn how to, you can check out the README. If not, you can wait until this is released as part of nightly build and provide any feedback then! |
Nice! Looking much better now! Let me know if this is what you're expecting. qemu-system-x86_64_AxK59KHxgO.mp4A couple quick thoughts...
Otherwise looking great! |
Yup, that's what I'm expecting! There's definitely some issues with this implementation at this time as I mentioned in the PR description.
I agree, we can think of a better way to show that something is a spoiler. What is implemented at the moment is mainly a placeholder since I was more focused on the overall logic first. I can add in an arrow to make it a bit more obvious. If you have any screenshots of how other apps do it, it would be great!
I can do that!
Could you clarify a bit more what you mean here? |
Well Boost doesn't support at all! 😆 Jerboa looks very much like the web (makes sense). scrcpy_aIgzmEiv8b.mp4Sync has an interesting implementation. It's not quite right, because it doesn't show the title before revealing, and there's no way to unreveal, but I do like the idea. It covers the spoiler in a solid block until revealed. I do think that makes it very clear that there's a spoiler. What do you think? scrcpy_17LnVvqFZ9.mp4
Hmm, well if you watch my last screen recording, you can see that each time I press on the spoiler (header or body), the whole comment would display the inkwell animation (making me concerned that the comment widget was also handling the tap gesture). But I can't repro, so I guess it's not a big deal! 😊 qemu-system-x86_64_gK7ZRWIK9D.mp4 |
|
Popping in here to say that I'm personally used to spoilers being a solid block until tapped (like Sync does, apparently). I do like the title idea though, that's pretty neat! Does "test spoiler title" mean the user is able to title their spoiler, or that's just a placeholder on our end? After some thought, I think that the solid block approach feels "simplest"/most straightforward to me, and has the extra benefit of being less likely to be misinterpreted as an external link (if we went with the spoiler turning into a title). Thoughts on that reasoning? |
|
Hope you're both well, also :) Some great Thunder updates the last couple of months (as always!) |
Yes exactly. Titles are part of the spoiler markdown specification. So in this example...
|
|
I haven't touched this feature much since the last update, but I'll work on it again soon! It seems like the general consensus here is to use a solid block to indicate spoilers. Maybe the first step to allowing spoiler support is to implement the solid block for spoilers with the assumption that there won't be additional markdown formatting within the spoiler itself (the spoiler content will just be plain text). Once that's completed, we can move a bit further and consider ways of improving rendering custom markdown within the spoiler itself. Thoughts? |
Yeah, that would be a great first step! Just having spoilers at all, even if the contents are (rarely) incorrectly formatted, would be a huge bonus! |
|
That makes sense to me! |
…/markdown-spoiler
…nder into feature/markdown-spoiler
…., links, images, etc)
|
@micahmo @machinaeZER0 I've updated the original description of the PR. Feel free to take a read through it and provide any feedback! @micahmo you can also test this out on Android to see if it's working as expected |
…/markdown-spoiler
There was a problem hiding this comment.
Hey I grabbed the latest and checked out the original post you made, and things seem to be working as designed! Great job on this! I approved the PR.
However, the more I thought about this, the more I thought expanding/collapsing is the way to go (and how important showing the title is). My reasoning here is that the number 1 way I've seen spoilers used on Lemmy is not for actual spoilers, but to collapse long content.
For example, here is the programming.dev sidebar. You can see they use spoilers as more of an design/organization/layout tool than anything else. And because that's how they're rendered in the web UI and Jerboa, it seems to be a very common use case.
firefox_lFGg39vWgB.mp4
Now that sidebar looks pretty funny in Thunder, where we (a) lose the heading and (b) have huge chunks of blank space, which is exactly what the authors wanted to avoid.
qemu-system-x86_64_FU1xZFwBMd.mp4
I'm sorry I didn't think of this use case before @hjiangsu put a ton of work into changing the implementation, but I'd be curious to hear what you and @machinaeZER0 think of this and whether it's worth doing something more like the original design.
|
Oh weird, I would never have assumed those were handled as spoilers! Very odd implementation, there 🤔 because of this very prominent example I'm inclined to agree with going with the title/expansion route, if only for feature parity in this particular use case! |
|
(That said, one could argue that their sidebar is "misusing" spoiler tags and that catering to their implementation doesn't necessarily follow what one might consider "best practices?" So I guess I could really go either way, honestly.) |
|
Thanks for the feedback @machinaeZER0. I totally agree with you that if were developing a true spoiler UI feature from scratch, it would be very different and almost certainly block-based. However, unfortunately it seems like people have latched onto the default UI implementation on the web and are using it much more as a content organization tool. And I'm afraid if we don't follow suit, we'll be the odd ones out. But hey, that's how these things work. People realize that can use something a certain way that works and looks cool, so they do! Also our implementation can be a little more elegant (maybe the title can look more like a dropdown button, maybe the content can expand with an animation, etc.). Here are a few more examples of instances/communities using this formatting in their sidebars (seems very common in the top communities). https://feddit.de/ |
|
Yeah, if the ship has sailed I think it only makes sense to fall in line 😄 |
|
I can go ahead and tweak it a bit to make it similar to how the web UI does it. I wonder though how other apps are handling this |
Here's a rundown of apps from https://join-lemmy.org/apps that I could test.
Note that my test case was to look at community sidebars, so if some of these say not supported (like Sync), it may work in post/comments. |
…/markdown-spoiler
…/markdown-spoiler
|
Just an update on this - I finally got around to changing the implementation to use expandable content rather than spoiler blocks like before. Here are some previews on how it looks like now, but @micahmo feel free to run it and let me know if this aligns better to how it's being used!
|
I think this looks amazing and is even an improvement on the default implementation! I'm glad that we can have our own flavor in Thunder that feels familiar but is unique! P.S. I noticed that it doesn't seem to work on the |
I just tried it and it seems to work. Is there a particular instance you have that I can test this on? Regardless, I think we can merge this in first, and then fix the edge cases that pop up since they might be more difficult to track down (especially with nested markdown)
|
You're right, it does! Not sure what I was seeing. Maybe I was confused with a community sidebar. For example, I see this on [email protected].
But as you said...
Completely agreed! |
Hmm, that may be a regex issue or possibly nested markdown as you mentioned. Regardless, I'll merge this in first and then look into this a bit more! |
Pull Request Description
This PR introduces some basic functionality for handling spoiler tags within Markdown. This works by creating a new
ExtensionSetwhich parses spoiler tags, and generates a custom widget for tags defined asspoilerthrough the use ofMarkdownElementBuilderThis implementation doesn't fully follow Lemmy's method of showing spoilers in that it doesn't show the title of the spoiler. However, most other things work as expected now. When there is a spoiler, it will hide it using a spoiler block. You can tap on the spoiler block to show or hide the spoiler.
Note: currently, the spoiler will take up roughly the same amount of space as the actual content of the spoiler. I don't know what the best way to do this is, so if there are any suggestions, that would be great!
When using Lemmy's spoiler tag definition, the spoiler will always be on a new line. This is because Lemmy's spoiler tag definition looks like:
I've also added unofficial support for doing inline spoiler tags as defined below. Note that this is not an officially supported specification/syntax so it does not work across Lemmy apps. If there ever comes a point where there's a way to define an inline spoiler text, then we can modify the regex to use that spec.
Issue Being Fixed
Issue Number: #304
Screenshots / Recordings
Checklist
semanticLabels where applicable for accessibility?