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

Thread summary is visually cut off when using maximized widget #20941

Closed
HarHarLinks opened this issue Feb 6, 2022 · 6 comments · Fixed by matrix-org/matrix-react-sdk#7838
Closed
Assignees
Labels
A-Appearance A-Threads A-Widgets O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Z-Labs Z-Maximised-Widgets

Comments

@HarHarLinks
Copy link
Contributor

Steps to reproduce

thread with maximized widget

Outcome

image

Operating system

arch

Application version

Element Nightly version: 2022020601 Olm version: 3.2.8

How did you install the app?

aur

Homeserver

No response

Will you send logs?

No

@SimonBrandner SimonBrandner added A-Appearance A-Threads O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist Z-Maximised-Widgets labels Feb 6, 2022
@github-actions github-actions bot added the Z-Labs label Feb 6, 2022
@germain-gg
Copy link
Contributor

@janogarcia iirc correctly we enforce a min-width for the thread summary.
What should happen in this scenario? We can go below the minimum recommended width and add an ellipsis? Does that sound like a fair thing to do here?

@janogarcia
Copy link

janogarcia commented Feb 7, 2022

@gsouquet Absolutely. We do enforce a minimum width when the summary content is short and there's plenty of available space in the container. But we could as well truncate it if there's not enough room.

That being said:

  • The specs for the thread summary that I shared were only accounting for the main timeline as its rendering context. Maximized widgets were introduced afterwards. So, I think I'll update the thread summary spec to account for that scenario, where the timeline container can get quite narrow .
  • @HarHarLinks's screenshot is showing an additional bug, where the right edge is cut off (notice the sharp right corners).

@HarHarLinks
Copy link
Contributor Author

I can confirm this happens everytime the timeline is narrow enough, maximized widgets are just the most common use case since nobody is going to use element like this:
image

@janogarcia
Copy link

janogarcia commented Feb 7, 2022

Thanks for the additional screenshot @HarHarLinks.

It also clearly shows how horribly broken the timeline is when narrowing the viewport without any constraints. We need to both improve the responsiveness of the message tile and set a sensible constraint for the minimum width of the timeline (anything below wouldn't be usable), but that's out of scope for this issue.

@gsouquet I've updated the spec for the thread summary to make its responsive behavior more robust.

Behavior for narrow timeline containers

When the timeline container gets too narrow (e.g. below 400px) the thread summary goes full width no matter its content and the user display name gets hidden.

Threads – Figma

https://www.figma.com/file/T309ztx0sNyOOK6NKVLHsK/Threads?node-id=1939%3A254160

@jryans
Copy link
Collaborator

jryans commented Feb 9, 2022

Seems straightforward enough, I've offered to pick this up from @gsouquet.

@janogarcia
Copy link

Heads up that I've updated the design to remove the label for the reply count when the timeline is rendered in a narrow container (below 400px).

Threads – Figma

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Appearance A-Threads A-Widgets O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Z-Labs Z-Maximised-Widgets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants