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

Usability fixes for new room header #11729

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Oct 11, 2023

Fixes a lot of UI feedback shared as part of https://www.figma.com/file/rTaQE2nIUSLav4Tg3nozq7/Compound-Web-Components?type=design&node-id=1609-8863&mode=design&t=hw2eZG1msBjOombN-11
Fixes element-hq/element-web#26346

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)

This change is marked as an internal change (Task), so will not be included in the changelog.

@germain-gg germain-gg added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Oct 11, 2023
@germain-gg germain-gg marked this pull request as ready for review October 11, 2023 10:49
@germain-gg germain-gg requested a review from a team as a code owner October 11, 2023 10:49
Comment on lines 49 to 51
/* Spacing of 64px between the topic and the buttons, but we have
to account for the gap of the flex items */
padding-right: var(--cpd-space-13x);
Copy link
Member

Choose a reason for hiding this comment

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

Something is a bit strange with a large gap growing at low widths

I don't see this gap in the Figma comps

image

Copy link
Member

Choose a reason for hiding this comment

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

The topic fills the space fine

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, you're very correct!
The padding ought to be applied to both the heading and the topic

Comment on lines 32 to 34
/* Spacing of 64px between the topic and the buttons, but we have
to account for the gap of the flex items */
padding-right: var(--cpd-space-13x);
Copy link
Member

@t3chguy t3chguy Oct 11, 2023

Choose a reason for hiding this comment

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

image

Based on the comment it looks like the wrong variable is used, or do you mean this is 64px - $gap where $gap seems to be 4px and the math still doesn't add up. Seems just like the comment is a little confusing, maybe supplying the formula for how you get from 64->52 would be helpful!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The gap on the flex parent is --cpd-space-3x which is 12px. It all adds up to 64px. Might cahnge the owrding of the comment

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.

The gap being so big in narrow windows looks like waste a lot of space but at least its consistent, lgtm other than comment clarity

@germain-gg germain-gg added this pull request to the merge queue Oct 11, 2023
Merged via the queue into develop with commit ac32d45 Oct 11, 2023
19 checks passed
@germain-gg germain-gg deleted the germain-gg/room-header-fixes branch October 11, 2023 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Facepile label split over 2 lines
2 participants