Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix timestamp and read receipt group placement on ThreadView #10335

Closed
wants to merge 33 commits into from
Closed

Fix timestamp and read receipt group placement on ThreadView #10335

wants to merge 33 commits into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 8, 2023

This PR changes style rules of the read receipts group and timestamp of hidden events so that they are displayed on the bubble layout on the thread, fixing the issue that read receipts overlapping save button of the editor on the modern layout (fixing partially, because the read receipts group will still overlap the button if it contains more than three or four avatars).

It looks the read receipts group has been enabled on the thread without taking the design compatibility into consideration pretty much, in order to make the thread implementation out of beta, so simply displaying it introduces a couple of issues, such as it being hidden by your own avatar on the bubble layout.

Also, for the modern layout, if you reserve horizontal space for the read receipt group to fix the issues element-hq/element-web#23576 and element-hq/element-web#23569 (comment), then it will create a similar issue as element-hq/element-web#21890 wasting a lot of horizontal space like this.

My question is: do we want to fix the obvious bugs at first, improving implementation step by step with other PRs, or do we want to improve the design of read receipts group on the thread and implement it as well with this PR to avoid the issues mentioned above from being introduced? I would prefer the former.

Before After
Thread panel Screenshot from 2023-03-09 04-59-00 Screenshot from 2023-03-09 04-59-05
Screenshot from 2023-03-10 17-57-44 Screenshot from 2023-03-10 17-56-43
Timestamp Screenshot from 2023-03-09 04-59-32 Screenshot from 2023-03-09 04-59-54
Read receipts Screenshot from 2023-03-09 05-02-45 Screenshot from 2023-03-09 05-02-55
Read receipts Screenshot from 2023-03-09 05-06-40 Screenshot from 2023-03-09 05-06-48
Read receipts (modern layout) Screenshot from 2023-03-13 18-41-09 Screenshot from 2023-03-13 18-42-02
MessageActionBar Screenshot from 2023-03-09 05-35-57 Screenshot from 2023-03-09 05-43-39
Editor (modern layout) Screenshot from 2023-03-09 06-29-37 Screenshot from 2023-03-09 06-28-49

Fixes element-hq/element-web#24773
Fixes element-hq/element-web#23867
Fixes element-hq/element-web#24744
Fixes element-hq/element-web#24808 with this commit
For element-hq/element-web#21774
Follow-up to #10284
For element-hq/element-web#23569
For element-hq/element-web#23326
For element-hq/element-web#23576

type: defect

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🐛 Bug Fixes

luixxiul added 13 commits March 9, 2023 05:08
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems labels Mar 8, 2023
Signed-off-by: Suguru Hirahara <[email protected]>
</div>,
msgOption,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand what these links were supposed to tell me. For the benefit of future reviewers:

This brings TimelineRenderingType.Thread in line with the other timeline rendering types by moving the msgOption (ie, the read receipt group) outside the mx_EventTile_line.

// Make sure the margin is NOT applied to mx_EventTile_line
cy.get(".mx_ThreadView .mx_GenericEventListSummary[data-layout=bubble]").within(() => {
cy.get(".mx_EventTile_info.mx_EventTile_last")
// 49px: --EventTile_bubble-margin-inline-start
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -1082,28 +1077,58 @@ $left-gutter: 64px;
top: 2px; /* Align with avatar */
}

.mx_ReadReceiptGroup {
inset-block-start: calc(-$font-22px); /* Align with hidden event content */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&.mx_EventTile_verified.mx_EventTile_info .mx_EventTile_line,
&.mx_EventTile_unverified.mx_EventTile_info .mx_EventTile_line,
&.mx_EventTile_unknown.mx_EventTile_info .mx_EventTile_line {
padding-inline-start: var(--ThreadView_group_spacing-start);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


&::before {
inset-inline: calc(-1 * var(--BaseCard_EventTile-spacing-inline));
inset-inline-start: calc(-1 * var(--BaseCard_EventTile-spacing-inline));
inset-inline-end: calc(-1 * var(--EventTile_bubble-margin-inline-end));
Copy link
Contributor Author

@luixxiul luixxiul Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't just link to another line in the diff. What am I supposed to learn by looking at that link?


&[data-self="true"] {
/* Align the avatar and mx_EventTile_line at the center on hidden event tile line */
align-items: center;
Copy link
Contributor Author

@luixxiul luixxiul Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.mx_EventTile_content,
.mx_RedactedBody {
margin-inline-start: calc(var(--ThreadView_group_spacing-start) + 14px + 6px);
/* Not available on the actual UI anyway as the hidden event expand button is hidden above */
Copy link
Contributor Author

@luixxiul luixxiul Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 49px: --EventTile_bubble-margin-inline-start
.should("have.css", "margin-inline-start", "49px");
cy.get(".mx_EventTile_info.mx_EventTile_last .mx_EventTile_line .mx_EventTile_content")
// Make sure the margin is NOT applied to mx_EventTile_content
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

luixxiul added 3 commits March 9, 2023 18:42
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 9, 2023

Found some other things to be fixed... working on them.

The declaration has been introduced with 73d346e introducing bubble layout in ThreadView. It has been required because '.mx_ThreadView .mx_EventTile' was styled with flexbox. The flexbox model was deprecated, and the declaration is no longer necessary.

Signed-off-by: Suguru Hirahara <[email protected]>
The declarations are specified by _EventBubbleTile.pcss, therefore they are not required here. display:block is not needed here.

Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
Signed-off-by: Suguru Hirahara <[email protected]>
@@ -1234,8 +1234,6 @@ $left-gutter: 64px;
}

&[data-self="true"] {
align-items: flex-end;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, what do I learn by looking at that link?

inset-inline-end: calc(
-1 * var(--ReadReceiptGroup_EventBubbleTile-spacing-end) + $container-gap-width + 6px
);
inset-inline-end: var(--BaseCard_EventTile_bubble_RR-inset-inline-end);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: https://github.com/matrix-org/matrix-react-sdk/pull/10335/files#diff-5d3e25eb1d22dd7a96e0e87ea9c1298dc7dbce8eb965d30b2d1ebc1436bfec3fR1230

Once more, just linking to a diff isn't very helpful.

In this case, this is a link to the other place that --BaseCard_EventTile_bubble_RR-inset-inline-end is now used.

@luixxiul
Copy link
Contributor Author

@andybalaam Would you please review when you have some time? Thanks in advance.

@andybalaam
Copy link
Member

My question is: do we want to fix the obvious bugs at first, improving implementation step by step with other PRs, or do we want to improve the design of read receipts group on the thread and implement it as well with this PR to avoid the issues mentioned above from being introduced? I would prefer the former.

Yes, 100% - I would prefer to review small changes that fix an obvious bug. Thank you for your work on this!

@andybalaam
Copy link
Member

I'm looking, but it's going to take me a while to get my head around all this, next week.

@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 10, 2023

No problem. It is a little bit large one and you seem to be busy with other tasks, so take your time 👍

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've started having a look at this, but there are quite a few things I don't understand. I have asked some questions below, though there are parts I haven't even looked at yet.

The changes have the air of being sensible, but they are hard to follow, and generally I wonder if this trying to fix too many things at once. Would it be possible to break this PR down into smaller increments to help us review it?

res/css/views/right_panel/_TimelineCard.pcss Outdated Show resolved Hide resolved
--BaseCard_EventTile_line-padding-block: 2px;
--BaseCard_EventTile-spacing-inline: 36px; /* TODO: Use a spacing variable */
--BaseCard_header-button-size: 24px;

/* Moving the read receipts group inside BaseCard */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should be in the imperative ("Move"), not the present participle ("Moving").

But in this case, the useful information is what this variable does, rather than how it is calculated.

Suggested change
/* Moving the read receipts group inside BaseCard */
/* The `inset-inline-end` (ie, `right` offset) for RR groups in bubble mode. */

@@ -14,9 +14,12 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

.mx_ReadReceiptGroup {
/* Required due to alphabetic order of CSS import */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do this elsewhere without a comment; I don't think the comment here is that helpful.

Suggested change
/* Required due to alphabetic order of CSS import */

@@ -14,9 +14,12 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

.mx_ReadReceiptGroup {
/* Required due to alphabetic order of CSS import */
:root {
--ReadReceiptGroup_EventBubbleTile-spacing-end: 78px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be good to add a comment saying what this variable does though. I'm struggling to figure it out. The best definition I came up with was:

Suggested change
--ReadReceiptGroup_EventBubbleTile-spacing-end: 78px;
/* The negative of the `inset-inline-end` of ReadReceiptGroups in bubble mode. */
--ReadReceiptGroup_EventBubbleTile-spacing-end: 78px;

Any better ideas?

(why isn't it just -78px, to save us doing calc(-1 * var(--ReadReceiptGroup_EventBubbleTile-spacing-end)) everywhere it is used?)

Comment on lines -1035 to -1036
display: flex;
flex-direction: column;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it safe to remove these?

Comment on lines -1053 to -1057
.mx_ViewSourceEvent_toggle {
display: none; /* hide the hidden event expand button, not enough
space, view source can still be used */
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is moving this necessary?

</div>,
msgOption,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand what these links were supposed to tell me. For the benefit of future reviewers:

This brings TimelineRenderingType.Thread in line with the other timeline rendering types by moving the msgOption (ie, the read receipt group) outside the mx_EventTile_line.


&::before {
inset-inline: calc(-1 * var(--BaseCard_EventTile-spacing-inline));
inset-inline-start: calc(-1 * var(--BaseCard_EventTile-spacing-inline));
inset-inline-end: calc(-1 * var(--EventTile_bubble-margin-inline-end));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't just link to another line in the diff. What am I supposed to learn by looking at that link?

@@ -1234,8 +1234,6 @@ $left-gutter: 64px;
}

&[data-self="true"] {
align-items: flex-end;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, what do I learn by looking at that link?

inset-inline-end: calc(
-1 * var(--ReadReceiptGroup_EventBubbleTile-spacing-end) + $container-gap-width + 6px
);
inset-inline-end: var(--BaseCard_EventTile_bubble_RR-inset-inline-end);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: https://github.com/matrix-org/matrix-react-sdk/pull/10335/files#diff-5d3e25eb1d22dd7a96e0e87ea9c1298dc7dbce8eb965d30b2d1ebc1436bfec3fR1230

Once more, just linking to a diff isn't very helpful.

In this case, this is a link to the other place that --BaseCard_EventTile_bubble_RR-inset-inline-end is now used.

@luixxiul
Copy link
Contributor Author

Thanks for the educational insights.

converting for now to make commits understandable for those who are not familiar with the style rules around EventTiles

@luixxiul luixxiul marked this pull request as draft March 22, 2023 12:42
@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 22, 2023

@richvdh Also I've felt your reviews aggressive and impolite, which is almost enough to have me give up working on this.

English lesson

Co-authored-by: Richard van der Hoff <[email protected]>
@richvdh
Copy link
Member

richvdh commented Mar 22, 2023

@richvdh Also I've felt your reviews aggressive and impolite, which is almost enough to have me give up working on this.

I'm sorry you feel that way. It's certainly not intended that way. To be clear: I (along with the rest of the team) am very grateful for all the great work you are doing on improving the application.

I know my review style can be a bit "blunt" and to the point. Ultimately I'm just trying to make sure we do our best to keep the codebase maintainable. But sometimes I am in a bit of a hurry and I forget that there is a human reading my comments. I will do my best to moderate my language.

Having said that: I do encourage you to look at your pull requests and think how you can make them easy for reviewers to understand. If you were doing a review yourself, without the benefit of having just worked on this bit of code, what would you find helpful? Sometimes that will mean making smaller PRs; sometimes it will mean adding comments to the diff which explain why you made a particular choice. I think that one reason this PR hasn't made progress more quickly is because it's a bit difficult to understand.

Anyway, to reiterate: thank you very much for all your work on the project. Please do keep it up!

@andybalaam
Copy link
Member

@luixxiul thank you again for this. I've finally got around to reviewing it again today, and I can't get my head around it.

Please can you break it down into small pieces that we can review one at a time? Thanks!

@andybalaam
Copy link
Member

I'm going to close this because we're not looking to merge it, but we really appreciate the work and would like to merge it when you can break it down into smaller parts.

@andybalaam andybalaam closed this May 9, 2023
@luixxiul luixxiul deleted the fix-timestamp-readreceipt-threadview-bubble-layout branch June 1, 2023 14:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
3 participants