[lib] put messagePreview function for mutlimedia and reaction message into respecitve message spec
ClosedPublic
Actions

Authored by ginsu on Dec 16 2022, 12:01 PM.

Details

Reviewers

atul
rohan
tomek

Commits

rCOMMe5094444d76e: [lib] put messagePreview function for mutlimedia and reaction message into…

Summary

Both multimedia and reaction messages were calling the messagePreview function. To keep the message specs more modular, we should move messagePreview into individual message specs. This was suggested by Ashoat in D5689

Linear Task: ENG-2393

Test Plan

Please watch the demo video to see that everything works as expected and there are no regressions:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Dec 16 2022, 12:01 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 16 2022, 12:01 PM

ginsu retitled this revision from [lib] put messagePreview function for mutlimedia and reactiion message into respecitve message spec to [lib] put messagePreview function for mutlimedia and reaction message into respecitve message spec.Dec 16 2022, 12:04 PM

ginsu edited the summary of this revision. (Show Details)

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, rohan, tomek.

ginsu attached a referenced file: F287386: Screen Recording 2022-12-16 at 2.55.01 PM.mov. (Show Details)

ginsu edited the test plan for this revision. (Show Details)

ginsu attached a referenced file: F287396: Screen Recording 2022-12-16 at 2.55.01 PM.mov. (Show Details)

Harbormaster completed remote builds in B14208: Diff 19441.Dec 16 2022, 12:14 PM

ginsu requested review of this revision.Dec 16 2022, 12:14 PM

Don't we need to make changes to lib/shared/messages/message-spec.js to accommodate this? Feel free to re-request review if I'm missing something

This revision now requires changes to proceed.Dec 18 2022, 9:40 PM

In D5886#177135, @atul wrote:

Don't we need to make changes to lib/shared/messages/message-spec.js to accommodate this? Feel free to re-request review if I'm missing something

I was thinking that this function would be a separate helper function in the message spec like shimMediaMessageInfo in the multimedia message spec, since messagePreviewText only gets called inside the message spec; however, if I am mistaken I can definitely implement it in the way you have described

Agree with @atul

lib/shared/messages/multimedia-message-spec.js
374 ↗	(On Diff #19441)	Helper function isn't needed, please inline this
379–380 ↗	(On Diff #19441)	Condition isn't needed
386 ↗	(On Diff #19441)	This case will never trigger?
lib/shared/messages/reaction-message-spec.js
212 ↗	(On Diff #19441)	Helper function isn't needed, please inline this
216 ↗	(On Diff #19441)	Condition isn't needed
224 ↗	(On Diff #19441)	This case will never trigger?