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

Read receipts improvements for thread view #9595

Closed
wants to merge 2 commits into from

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Nov 18, 2022

Fixes element-hq/element-web#23569
Fixes element-hq/element-web#23576

Screenshot 2022-11-18 at 09 23 58

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

@germain-gg germain-gg added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Nov 18, 2022
@germain-gg germain-gg requested a review from a team as a code owner November 18, 2022 09:27
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'm afraid I'm really struggling to understand what the CSS changes are doing, and why. Maybe you could add a couple of comments to the diff to help me understand?

</div>,
reactionsRow,
msgOption,
Copy link
Member

Choose a reason for hiding this comment

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

Some grumbling about code style which I don't particularly expect you to fix: why is this called msgOption when it only contains the read receipts? Also 600 line functions suck.

</div>,
reactionsRow,
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 note that the non-threads case does:

React.createElement("li", <attrs>, 
<>
  { elem }
  { elem }
</>);

whereas this bit does

React.createElement("li", <attrs>, 
[
  elem,
  elem,
]);

Is there a difference? or any reason to prefer one style over the other?

Comment on lines +138 to 142
.mx_EventTile:not(:last-child) {
.mx_ReadReceiptGroup_noReceipts {
display: none;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

could you give this a comment? Evidently it's trying to hide the read receipt group under certain conditions, but I'm failing to grok what those conditions are

@@ -106,7 +106,7 @@ limitations under the License.
content-visibility: visible;
}

.mx_EventTile,
.mx_EventTile[data-layout],
Copy link
Member

Choose a reason for hiding this comment

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

when is data-layout not present, and what's the rationale for doing this with an attribute selector rather than a class?

@@ -353,7 +353,7 @@ export default class ThreadView extends React.Component<IProps, IState> {
ref={this.timelinePanel}
showReadReceipts={true}
manageReadReceipts={true}
manageReadMarkers={true}
manageReadMarkers={false}
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering what this actually did, but the comment just says "Enable managing RRs and RMs." Well gee thanks.

What does managing actually entail? and why do we now want to turn it off?

margin-bottom: 0;
}

.mx_EventTile_msgOption {
Copy link
Member

@richvdh richvdh Nov 18, 2022

Choose a reason for hiding this comment

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

again, is this just used for read receipts? A comment might help if so.

@germain-gg
Copy link
Contributor Author

Punted to a later stage, this has gone stale and does not match what the design team was interested in.

@germain-gg germain-gg closed this Dec 14, 2022
@germain-gg germain-gg deleted the gsouquet/rr-overlap-23569 branch December 14, 2022 10:14
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read receipts in threads overlap interactive elements Read receipts overlapping text in a thread
2 participants