Having the replyButton in MessageActionButtons instead of ComposedMessage sets things up for later diffs...
Details
Details
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
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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? |
| 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 |
| 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) | |
| 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) |
| 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? |
| web/chat/composed-message.react.js | ||
|---|---|---|
| 150–154 ↗ | (On Diff #9969) |
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. |
| web/chat/message-action-buttons.js | ||
|---|---|---|
| 144–148 ↗ | (On Diff #9969) | |