-
Notifications
You must be signed in to change notification settings - Fork 72
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
Support threads in Matrix (MSC3440) #668
Conversation
Known issues (I'd like to file these as issues (if not already filed) and solve in different PRs):
|
Can we do a dumb thing where we just send the event twice to Matrix? Would this be too spammy? Alternatively should we comment on the proposal to raise the gap in the spec?
I'd be reasonably okay with leaving this as an open issue to fix, if it's not an easy fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks super clean and appears to work, nice one!
src/SlackGhost.ts
Outdated
}, | ||
}, | ||
"msgtype": "m.text", // for those who just want to send the reply as-is | ||
"body": `${fallbackText}\n\n${this.prepareBody(text)}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Element does not provide this fallback.
I guess if a client or bridge (e.g. IRC) wants to display the relation, it up to that cleint's or bridge's implementation to provide an adequate representation.
Co-authored-by: Will Hunt <[email protected]>
…service-slack into j94/matrix-threads
Fixes #634
Note: Slack doesn't seem to have a separate UI for replies. Everything is a thread.
Definition of Done
m.thread
relations – removing the reply relation we currently set.