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

Fix thread summary layout for narrow right panel timeline #7838

Merged
merged 9 commits into from
Feb 23, 2022

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Feb 17, 2022

image

There are still some issues in the bubble version of the right panel chat timeline, but it has more layout issues than just threads, so seems better to tackle it separately.

Fixes element-hq/element-web#20941


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://pr7838--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.

@jryans jryans requested a review from a team as a code owner February 17, 2022 17:53
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.

This request might be a bit more involved, as it's no longer a CSS-only change, but would it be possible to hide the label as well for the reply count (e.g., 3 instead of 3 replies) when the timeline is rendered in a narrow container (below 400px)?

Threads – Figma

@jryans
Copy link
Collaborator Author

jryans commented Feb 18, 2022

The "replies" label is now hidden for narrow (<= 400 px) timelines.

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.

@jryans Thanks, Ryan, for updating it! Just one remark, it seems the min-width used for 400px+ timelines is still being applied for narrow ones. As per the design mockup, we should remove it for timelines below 400px, letting it adjust itself to the available space.

@jryans
Copy link
Collaborator Author

jryans commented Feb 18, 2022

@janogarcia Hmm, are you looking at a normal chat timeline, or the right panel style used with maximised widgets? I don't notice any min-width for the right panel style:

right-panel-thread-summary

At the moment in this PR, I have just forced the right panel timeline used with maximised widgets to present the narrow version, as a responsive room timeline has many other more severe problems than this thread summary, so I thought it was perhaps best approached as it's own future project. (Maybe a bad assumption on my side!) 😅

Anyway, if you do want this to be truly responsive across all types of timelines, I can change to that. At the moment, we effectively don't make any attempt at a responsive timeline, so the thread summary would be one of the few places that tries for now.

@jryans jryans removed the request for review from t3chguy February 18, 2022 18:01
@janogarcia
Copy link
Contributor

janogarcia commented Feb 18, 2022

@jryans Yeah, I did test it in the main timeline (which, as you pointed out, has all sorts of problems with responsiveness). I'm all for making this kind of component responsive to whatever context they are dropped in (a la CSS Container Queries) rather than tweaking its responsive behavior on a case-by-case basis. It should be easier to maintain and more future-proof.

So, yes, that would be a super welcome enhancement. 👍

@jryans
Copy link
Collaborator Author

jryans commented Feb 21, 2022

Okay, this should switch between wide and narrow modes responsively in both the main and right panel timelines. Here's the main timeline:

main-timeline-thread-summary

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

src/components/structures/MessagePanel.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/EventTile.tsx Outdated Show resolved Hide resolved
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.

Okay, this should switch between wide and narrow modes responsively in both the main and right panel timelines. Here's the main timeline:

main-timeline-thread-summary

@jryans Thanks for the update, Ryan. Just one question, does the thread summary expands to 100% of the available space when in narrow container mode? In the GIF, it seems as if the size suddenly jumps to 100% when switching to the non-narrow one.

@janogarcia
Copy link
Contributor

I forgot about the Netlify test deploy. I'll test it myself. 🙂👍

@janogarcia janogarcia self-requested a review February 21, 2022 18:22
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.

Just tested it, and it works as expected. Thanks!

@jryans
Copy link
Collaborator Author

jryans commented Feb 22, 2022

This version uses a single breakpoint tested from a single place as discussed above and passes the result via the context.

@jryans jryans requested a review from t3chguy February 22, 2022 16:56
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Generally sane otherwise

src/components/views/elements/Measured.tsx Outdated Show resolved Hide resolved
@jryans jryans merged commit d8ac7cf into develop Feb 23, 2022
@jryans jryans deleted the jryans/thread-summary-width branch February 23, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thread summary is visually cut off when using maximized widget
3 participants