Rich message rendering #24

Merged
cadence merged 28 commits from rich-messages into princess 2020-11-07 10:46:48 +00:00
Collaborator

I don't know tbh, someone smarter look at this.

This adds support for rendering rich messages and creates what I think is a good way to handle events. This still needs work and even if we decide not to use this someone else can probably reuse code from this

I don't know tbh, someone smarter look at this. This adds support for rendering rich messages and creates what I think is a good way to handle events. This still needs work and even if we decide not to use this someone else can probably reuse code from this
cadence reviewed 2020-10-29 07:46:58 +00:00
cadence left a comment
Owner

Just going through a visual analysis of your code, I'll test it and give a full review in the future.

Just going through a visual analysis of your code, I'll test it and give a full review in the future.
@ -0,0 +13,4 @@
// predicates
canGroup() {
Owner

The original idea here was that we'd have a predicate for things like whether the method can be displayed as a message group, whether it should show a timestamp, or whatever other things could affect the display based on the class. I haven't actually put this method to any use yet.

The original idea here was that we'd have a predicate for things like whether the method can be displayed as a message group, whether it should show a timestamp, or whatever other things could affect the display based on the class. I haven't actually put this method to any use yet.
@ -0,0 +50,4 @@
}
}
function simpleEvent(filter, render) {
Owner

Not sure how I feel about a function that creates classes. Can we inherit from a base class instead?

Not sure how I feel about a function that creates classes. Can we inherit from a base class instead?
Author
Collaborator

That's a lot more verbose, which I don't really like, but sure

That's a lot more verbose, which I don't really like, but sure
@ -0,0 +3,4 @@
const UnknownMembership = simpleEvent((e) => e.type == "m.room.member", (e) => "unknown membership event")
function createMembershipEvent(membership, message) {
return simpleEvent((e) => e.type == "m.room.member" && e.content.membership === membership, message)
Owner

Again here. I think a better way would be to have a base class like MembershipEvent, and then for each kind of membership we subclass it and only implement the canRender method on each.

Again here. I think a better way would be to have a base class like MembershipEvent, and then for each kind of membership we subclass it and only implement the canRender method on each.
bad marked this conversation as resolved
@ -0,0 +25,4 @@
}
})
//Handle our custom tag
Owner

style: can we put spaces after // ?

style: can we put spaces after // ?
Author
Collaborator

To consider: highlight.js is thicc. It's almost 1MiB. Not sure if bundle size is something we care about but we should probably keep this in mind...

To consider: highlight.js is thicc. It's almost 1MiB. Not sure if bundle size is something we care about but we should probably keep this in mind...
Owner

1 MB bundle size is kind of ridiculous. We could simply not highlight code blocks, or we could maybe inline async import the module on demand when we actually need to do highlighting, so it's out of the initial bundle.

1 MB bundle size is kind of ridiculous. We could simply not highlight code blocks, or we could maybe inline async import the module on demand when we actually need to do highlighting, so it's out of the initial bundle.
Author
Collaborator

Added lazy loading for highlight.js. It's a bit hacky and uses an eval() but it gets the job done

Added lazy loading for highlight.js. It's a bit hacky and uses an eval() but it gets the job done
cadence approved these changes 2020-11-05 05:04:30 +00:00
bad changed title from WIP: Rich message rendering proposal to Rich message rendering 2020-11-05 17:05:03 +00:00
cadence merged commit f6b95b2ebd into princess 2020-11-07 10:46:47 +00:00
Sign in to join this conversation.
No description provided.