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

Adding spoiler tag support #990

Merged
merged 18 commits into from
Jul 11, 2023
Merged

Adding spoiler tag support #990

merged 18 commits into from
Jul 11, 2023

Conversation

ZJouba
Copy link
Contributor

@ZJouba ZJouba commented Jul 7, 2023

Addressing: #627
Adding ::: spoiler tag support for Jerboa

Spoiler.Tag.mp4

Nested tags are not supported and parsing can be weird if the syntax isn't correct
I'll wait for #734 to be merged, update and then put mine up for review

Post used for testing: https://voyager.lemmy.ml/post/1053

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 7, 2023

Looks great but I would probably put that class SpoilerPlugin in Utils

@twizmwazin
Copy link
Contributor

I disagree, we already have a markwon plugin in this file and we shouldn't be dumping everything in Utils if there is a better place for it. If the custom plugins become too much to contain in this file, it'd be better to make a package for them.

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 8, 2023

I don't mean the utils.kt, the package Utils as a file. Common package already contains too many non composables that aren't used commonly. When my PRs get merged I look into getting Account & navigation stable. And I'll move nav out of common into a proper package.

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 10, 2023

@MV-GH I hope I understood you correctly with this change 🤣

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested this, and it works great, but unfortunately the click to collapse comment overrides it. Although you can long-click to reveal the spoiler.

@MV-GH @twizmwazin I'm not sure if we should merge as is, and tackle that one separately.

@dessalines dessalines requested a review from MV-GH July 10, 2023 19:53
@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 10, 2023

Hey @dessalines, thanks for the 👀

I'm happy to tackle it pronto, I just need a teensy bit of guidance. Does Compose have a 'preventDefault` option?

@twizmwazin
Copy link
Contributor

twizmwazin commented Jul 10, 2023

Is the issue that tapping to reveal conflicts with tapping to collapse a comment? If so, the desired behavior I think is that for text that is intractable, such as a spoiler, that text's behavior should take precedence over collapsing the comment. There is space in the comment header that will always be empty/non-interactable to collapse a comment even if the entire body is interactable. For implementation, it would be intuitive to me if setting Modifier.clickable(), then that composable should consume the click event rather than the parent. If that's not the case then I'm unsure.

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 11, 2023

The spoiler tag is only a ClickableSpan and the entire comment body has the toggleExpanded on click listener. I'm not sure how to have it stop after the spoiler. Because both onClicks get called. Its not disregarding the spoiler onClick

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 11, 2023

Oops, didn't see #695


val wrapper = object : ClickableSpan() {
override fun onClick(p0: View) {
textView.cancelPendingInputEvents()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it's as easy as this

@ZJouba ZJouba marked this pull request as ready for review July 11, 2023 11:24
@ZJouba ZJouba requested a review from twizmwazin as a code owner July 11, 2023 11:24
Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 11, 2023

studio64_2SBC6yGk9b.mp4

Spoilers are clickable in preview mode, imo they shouldnt

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 11, 2023

If everyone agrees, it's easy to remove

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 11, 2023

I am pretty sure the others will agree with this. What approach were you going to take for this?

@ZJouba
Copy link
Contributor Author

ZJouba commented Jul 11, 2023

I can add a preview boolean to the plugin which is set when previewMarkwon is used which will remove the onClick functionality

Preview-Non-Clickable.mp4

Copy link
Contributor

@twizmwazin twizmwazin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once interaction is disabled for preview. Your approach sounds good.

@twizmwazin twizmwazin merged commit 0f1f2ce into LemmyNet:main Jul 11, 2023
@ZJouba ZJouba deleted the issue/627 branch July 12, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants