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

[MM-57266] UX improvements to call card (post component) #675

Merged
merged 10 commits into from
Apr 9, 2024

Conversation

streamer45
Copy link
Collaborator

Summary

PR implements improvements as suggested in the design file

@matthewbirtch One bit missing here is the row divider in "narrow" view. This is because it's a bit tricky to change layout dynamically as we'd need to show an element that is not there naturally. It can be done but it may end up being hacky as CSS would handle any wrapping naturally when necessary. But let me know how important this is and whether we should reconsider.

@cpoile On the above, let me know if you have any ideas but the only solution I could think of so far would be to hook up a ResizeObserver and dynamically check on div width to render the divider but it's probably going to be a perf nightmare.

Screenshots

image

image

image

image

Ticket Link

https://mattermost.atlassian.net/browse/MM-57266

@streamer45 streamer45 added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer labels Apr 3, 2024
@streamer45 streamer45 self-assigned this Apr 3, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.05%. Comparing base (fb8a905) to head (8731f45).

Additional details and impacted files
@@                  Coverage Diff                  @@
##           styled-components-v6     #675   +/-   ##
=====================================================
  Coverage                 11.05%   11.05%           
=====================================================
  Files                        27       27           
  Lines                      5690     5690           
=====================================================
  Hits                        629      629           
  Misses                     5003     5003           
  Partials                     58       58           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cpoile
Copy link
Member

cpoile commented Apr 4, 2024

Would css container queries work? I'm not sure what browsers we have to support (it's newer) (https://caniuse.com/css-container-queries).
Since we can assume the card will wrap at a certain viewport width, could we use a media query event?
This is useful: https://tigeroakes.com/posts/resize-event-alternatives/

@streamer45
Copy link
Collaborator Author

Would css container queries work? I'm not sure what browsers we have to support (it's newer) (https://caniuse.com/css-container-queries).

Thanks! Let me look into that.

Since we can assume the card will wrap at a certain viewport width, could we use a media query event? This is useful: https://tigeroakes.com/posts/resize-event-alternatives/

It's not really the viewport the main issue, but the fact the component renders in RHS and RHS is expansible so it's a bit more tricky. Unless of course we wrapped when in RHS always but that would be a big generalization.

@streamer45
Copy link
Collaborator Author

@cpoile Container queries seem to be exactly what we require here! The only problem now is that we need to bump styled components to make them work (from v5 to v6) which may turn out to be a bit of a pain, getting all sorts of errors. Will keep you posted.

`;

const SubMessage = styled.div`
white-space: normal;
font-size: 12px;
line-height: 16px;
color: rgba(var(--center-channel-color-rgb), 0.72);
`;

const Profiles = styled.div`
Copy link
Contributor

Choose a reason for hiding this comment

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

Add here as well

&:empty {
    display: none;
}

Copy link
Contributor

@matthewbirtch matthewbirtch left a comment

Choose a reason for hiding this comment

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

This is looking great. Thanks for the work on it @streamer45. A few minor things from me:

  1. In the "Call ended" state, in the RHS there is extra space at the bottom of the card. It's likely because the right content container is still present, but empty and the gap is added.
    image

  2. For the divider, it would be great if we can add it somehow as it helps make it look more organized when it wraps. If we are able to do @container queries so that the border and gap shows only when the component is below 400px (roughly) that could work.

  3. Can we reduce the gap between the avatars and the button?
    Screenshot 2024-04-05 at 12 22 53 PM

@streamer45 streamer45 changed the base branch from main to styled-components-v6 April 5, 2024 22:49
@streamer45
Copy link
Collaborator Author

Rebased and applied the changes (using @container queries now). Should look better now.

@matthewbirtch
Copy link
Contributor

Thanks @streamer45. Looks great now. Just one minor comment. Should be good to go once that's updated.

Copy link
Contributor

@matthewbirtch matthewbirtch left a comment

Choose a reason for hiding this comment

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

Thanks @streamer45

@streamer45 streamer45 removed the 1: UX Review Requires review by ux label Apr 8, 2024
@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Apr 9, 2024
@streamer45 streamer45 merged commit 41fe7fa into styled-components-v6 Apr 9, 2024
6 checks passed
@streamer45 streamer45 deleted the MM-57266 branch April 9, 2024 20:51
streamer45 added a commit that referenced this pull request Apr 11, 2024
* Migrate to styled components v6

* [MM-57266] UX improvements to call card (post component) (#675)

* UX improvements to call card

* Proper wrapping

* Language update

* Fixes after icon update

* Update snapshots

* Update e2e test

* Implement narrow layout

* Update e2e snapshots

* Fix row gap

* Fix divider's border
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants