Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply tweaks to Thread list as per design spec #8149

Merged
merged 23 commits into from Apr 5, 2022
Merged

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Mar 25, 2022

For element-hq/element-web#21502
Fixes element-hq/element-web#21538
Fixes element-hq/element-web#21472

image
image
image
image
image
image
image
image


This PR currently has no changelog labels, so will not be included in changelogs.

Add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is plus X-Breaking-Change if it's a breaking change.

Preview: https://pr8149--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #8149 (7493935) into develop (371ccd7) will decrease coverage by 0.00%.
The diff coverage is 35.00%.

@@             Coverage Diff             @@
##           develop    #8149      +/-   ##
===========================================
- Coverage    28.69%   28.68%   -0.01%     
===========================================
  Files          851      851              
  Lines        49779    49788       +9     
  Branches     12658    12662       +4     
===========================================
  Hits         14282    14282              
- Misses       35497    35506       +9     
Impacted Files Coverage Δ
src/components/views/messages/TextualBody.tsx 57.29% <0.00%> (ø)
src/components/structures/ThreadPanel.tsx 25.71% <12.50%> (-1.98%) ⬇️
src/components/views/rooms/EventTile.tsx 46.36% <44.44%> (-0.25%) ⬇️
...ponents/views/context_menus/MessageContextMenu.tsx 54.83% <100.00%> (-0.29%) ⬇️

@t3chguy t3chguy marked this pull request as ready for review March 30, 2022 11:13
@t3chguy t3chguy requested a review from a team as a code owner March 30, 2022 11:13
@t3chguy t3chguy requested a review from janogarcia March 30, 2022 11:13
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Code looks great 👍

Copy link
Contributor

@janogarcia janogarcia 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 unchecked and added notes to all tasks that need some attention:
element-hq/element-web#21502 (comment)

Update the truncating behavior for the thread summary:
element-hq/element-web#21472 (comment)

@germain-gg germain-gg merged commit 27e4806 into develop Apr 5, 2022
@germain-gg germain-gg deleted the t3chguy/fix/21502 branch April 5, 2022 16:01
@janogarcia
Copy link
Contributor

janogarcia commented Apr 5, 2022

Already reported/handled internally:

  • 🐛 Regression: Right border of container should be 8px (instead of 14px).
  • 🐛 Not fixed yet: Barely visible, the dot is almost completely cut off

thread panel

cropped-unread-dots

@karlicoss
Copy link

Hi, sorry if it's not the right place to ask, but figured it's the MR where this behavior was introduced.
What's the reason for truncating the width of the thread summary?

image

Gone through the linked issues, but not sure I understand the max-width restriction in particular. There is enough space to fit much more in the preview.
I've undone the max-width locally with Stylus extension in the meantime:

@-moz-document domain("app.element.io") {
    .mx_ThreadSummary {
       max-width: unset !important;
    }
}

image

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants