Page MenuHomePhabricator
Differential D3309

[web] Conditionally lay out `MessageActionButtons` based on `isViewer`
ClosedPublic
Actions

Authored by atul on Feb 28 2022, 3:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 5:49 AM
Unknown Object (File)
Fri, Nov 8, 12:50 PM
Unknown Object (File)
Sat, Nov 2, 8:41 AM
Unknown Object (File)
Tue, Oct 22, 11:42 PM
Unknown Object (File)
Oct 7 2024, 12:05 PM
Unknown Object (File)
Oct 7 2024, 12:05 PM
Unknown Object (File)
Oct 7 2024, 12:05 PM
Unknown Object (File)
Oct 7 2024, 12:05 PM
View All Files
Subscribers
• adrian
ashoat
• karol

Details

Reviewers
varun
• benschac
tomek
• jacek
ashoat
Commits
rCOMM3ecb605a9b54: [web] Conditionally lay out `MessageActionButtons` based on `isViewer`
Summary

Match existing layout (reply button closer to message)... fixes issue knowingly "introduced" in D3306

Test Plan

!isViewer: https://blob.sh/atul/400a.png
isViewer: https://blob.sh/atul/7c56.png

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Feb 28 2022, 3:33 PM
Herald added subscribers: • karol, • adrian, ashoat. · View Herald TranscriptFeb 28 2022, 3:33 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 28 2022, 3:36 PM
Harbormaster failed remote builds in B7076: Diff 9967!
Harbormaster completed remote builds in B7076: Diff 9967.Feb 28 2022, 4:02 PM
atul requested review of this revision.Feb 28 2022, 4:02 PM
tomek requested changes to this revision.Mar 1 2022, 3:33 AM
tomek added inline comments.
web/chat/message-action-buttons.js
174–183 ↗(On Diff #9967)

I think we could as well conditionally set a different css class which uses flex-direction: row-reverse. That would reduce the duplication... what do you think? If you think the current approach is cleaner, we can keep it.

This revision now requires changes to proceed.Mar 1 2022, 3:33 AM
atul added a comment.Mar 1 2022, 5:34 AM

I think we could as well conditionally set a different css class which uses flex-direction: row-reverse. That would reduce the duplication...

That sounds like a way better/cleaner approach, I’ll update this diff with that change

atul updated this revision to Diff 10112.Mar 7 2022, 7:27 AM

address feedback (@palys-swm thanks for suggesting row-reverse, this is a lot cleaner)

atul updated this revision to Diff 10113.Mar 7 2022, 7:28 AM

fix

ashoat added a comment.Mar 7 2022, 7:29 AM

row-reverse results in some weird behavior when selecting text with the cursor, can you test to make sure there are no weird regressions here?

Harbormaster completed remote builds in B7183: Diff 10112.Mar 7 2022, 7:32 AM
atul added a comment.EditedMar 7 2022, 7:32 AM

row-reverse results in some weird behavior when selecting text with the cursor, can you test to make sure there are no weird regressions here?

Ah thanks for flagging that, I'll do some research and then also do some manual testing before landing

edit: I think it's fine for now since we only have icons within this container, but we may need to make adjustments if that changes?

Harbormaster completed remote builds in B7184: Diff 10113.Mar 7 2022, 7:35 AM
tomek accepted this revision.Mar 8 2022, 7:16 AM
tomek added a reviewer: ashoat.
In D3309#90411, @atul wrote:

row-reverse results in some weird behavior when selecting text with the cursor, can you test to make sure there are no weird regressions here?

Ah thanks for flagging that, I'll do some research and then also do some manual testing before landing

edit: I think it's fine for now since we only have icons within this container, but we may need to make adjustments if that changes?

For me it makes sense. We have a lot more important issue with reverse - when selecting text from a couple of messages. We have a task for it created a long time ago https://www.notion.so/commapp/Handling-multiline-select-on-web-8374bbd134cb424e999edadc4a3deb4c. This one is a lot less important.

web/chat/message-action-buttons.css
6

This is a default value, so we could as well remove this class.

ashoat accepted this revision.Mar 8 2022, 9:25 AM

It would be great if somebody could move that task to Linear!

This revision is now accepted and ready to land.Mar 8 2022, 9:25 AM
atul added a comment.Mar 8 2022, 10:40 AM
In D3309#90942, @ashoat wrote:

It would be great if somebody could move that task to Linear!

https://linear.app/comm/issue/ENG-845/properly-handle-multiline-select-on-web

atul added inline comments.Mar 8 2022, 10:42 AM
web/chat/message-action-buttons.css
6

I'd prefer to keep it to be explicit

Particularly in message-action-button:

const messageActionButtonsContainer = classNames({
  [css.messageActionButtons]: true,
  [css.messageActionButtonsViewer]: isViewer,
  [css.messageActionButtonsNonViewer]: !isViewer,
});

Can change this if there's a strong opinion

atul updated this revision to Diff 10172.Mar 8 2022, 10:45 AM

rebase before landing

Harbormaster completed remote builds in B7228: Diff 10172.Mar 8 2022, 10:50 AM
Closed by commit rCOMM3ecb605a9b54: [web] Conditionally lay out `MessageActionButtons` based on `isViewer` (authored by atul). · Explain WhyMar 8 2022, 10:51 AM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM3ecb605a9b54: [web] Conditionally lay out `MessageActionButtons` based on `isViewer`.
tomek added inline comments.Mar 9 2022, 2:40 AM
web/chat/message-action-buttons.css
6

Agree, there's a value in having this explicitly stated, so we can keep the current approach

Revision Contents
Changeset List

Diff 10113

View Options

web/chat/message-action-buttons.css

Loading...
View Options

web/chat/message-action-buttons.js

Loading...