Rich message rendering #24

bad · 2020-10-26T22:18:20Z

bad commented

2020-10-26 22:18:20 +00:00

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

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

src/js/events/event.js

					
				@ -0,0 +13,4 @@

					// predicates

					canGroup() {

cadence commented

2020-10-29 07:46:24 +00:00

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.

src/js/events/event.js Outdated

					
				@ -0,0 +50,4 @@

					}

				}

				function simpleEvent(filter, render) {

cadence commented

2020-10-29 07:35:39 +00:00

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

bad commented

2020-10-29 10:07:08 +00:00

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

src/js/events/membership.js Outdated

					
				@ -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)

cadence commented

2020-10-29 07:45:04 +00:00

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

src/js/events/message.js Outdated

					
				@ -0,0 +25,4 @@

					}

				})

				//Handle our custom tag

cadence commented

2020-10-29 07:42:57 +00:00

style: can we put spaces after // ?

bad commented

2020-10-30 22:14:08 +00:00

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...

cadence commented

2020-10-30 23:49:15 +00:00

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.