Details

Reviewers

Commits

Summary

Having the replyButton in MessageActionButtons instead of ComposedMessage sets things up for later diffs...

Test Plan

Things continue to work as they did before.
Two known issues that will be addressed in subsequent diffs (wanted to limit the scope of this diff)

We want the icons to appear next to each other (flex-direction:row) instead of "on top" of each other
We want to position the action buttons (sidebar and reply) conditionally based on isViewer

Diff Detail

Repository

rCOMM Comm

Branch

landmarch8 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

atul requested review of this revision.Feb 28 2022, 3:19 PM

fix invariant message

remove unnecessary invariant

tomek requested changes to this revision.Mar 1 2022, 3:14 AM

tomek added inline comments.

web/chat/composed-message.react.js
131 ↗	(On Diff #9969)
150–154 ↗	(On Diff #9969)	If a class should be always added, you don't need to include it in the object
web/chat/message-action-buttons.js
45 ↗	(On Diff #9969)	Does this need to be optional?
144–148 ↗	(On Diff #9969)	There are a lot of invariants in this diff, which makes me quite uncomfortable. We can make our type declarations here safer, without having to use invariants. So the idea is to introduce a new prop, e.g. `messageState` that would be defined as: { canReply: false } \| { canReply: true, setMouseOverMessagePosition: (...) => void, mouseOverMessagePosition: OnMessagePositionWithContainerInfo...} That would mean that either we can't reply, or we can and need to provide all the functions / props. What do you think?

This revision now requires changes to proceed.Mar 1 2022, 3:14 AM

web/chat/composed-message.react.js
150–154 ↗	(On Diff #9969)	Personally I think it's cleaner to include it in the object, but I don't feel strongly

atul requested review of this revision.Mar 7 2022, 6:49 AM

atul marked 5 inline comments as done.

atul added inline comments.

web/chat/composed-message.react.js
150–154 ↗	(On Diff #9969)	I'd prefer keeping it as is, just because I like the symmetry and think it's a bit nicer to read the way things are formatted. But, if the idiomatic thing to do is pull "always true" classes out of the object, I'm happy to make that change.
web/chat/message-action-buttons.js
45 ↗	(On Diff #9969)	It does for now because `MessageActionButtons` is also used for robotext messages. I wanted to keep this as "light" of a refactor diff as possible, so I "threw in" all of the props needed to make things work. Later diffs (eg D3310 and D3311) clean things up, deduplicate props, etc.
144–148 ↗	(On Diff #9969)	You're right, there are more invariants in this diff than there need to be. I worked on cleaning up the props in later diffs (eg D3310 and D3311) so rebasing and changing things here would be hard. I'll work on using the type system rather than hacking in invariants later in the stack (at least after D3310 & D3311)

address feedback

tomek added inline comments.

web/chat/composed-message.react.js
150–154 ↗	(On Diff #9969)	I slightly prefer having "always true" classes outside, because for the case where there are only "always true", it doesn't make much sense to have an object with all the values set to true. But also agree that this symmetric approach makes sense. So what do you think about always using object unless all the classes are "always true"?
web/chat/message-action-buttons.js
45 ↗	(On Diff #9969)	Ok, makes sense!
144–148 ↗	(On Diff #9969)	Thanks! Are you planning to do it immediately or prefer having a linear task?

This revision is now accepted and ready to land.Mar 8 2022, 7:08 AM

web/chat/composed-message.react.js
150–154 ↗	(On Diff #9969)	So what do you think about always using object unless all the classes are "always true"? That makes sense!
web/chat/message-action-buttons.js
144–148 ↗	(On Diff #9969)	I'm going to work on it today after landing what's accepted. I have it written down to make a Linear task if I don't get to it by end of day.