Rich message rendering #24

Merged
cadence merged 28 commits from rich-messages into princess 3 years ago
bad commented 3 years ago
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 3 years ago
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?
bad commented 3 years ago
Poster
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 // ?
bad commented 3 years ago
Poster
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.
bad commented 3 years ago
Poster
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 3 years ago
bad changed title from WIP: Rich message rendering proposal to Rich message rendering 3 years ago
cadence merged commit f6b95b2ebd into princess 3 years ago

Reviewers

cadence approved these changes 3 years ago
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
The pull request has been merged as f6b95b2ebd.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b rich-messages princess
git pull origin rich-messages

Step 2:

Merge the changes and update on Forgejo.
git checkout princess
git merge --no-ff rich-messages
git push origin princess
Sign in to join this conversation.
Loading…
There is no content yet.