-
Notifications
You must be signed in to change notification settings - Fork 57
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-57263] Tweaks to expanded view #715
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some non-blocking comments. Thanks Claudio!
style={{ | ||
width: '24px', | ||
height: '24px', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be 20 x 20 now, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I got confused once more.
className='button-close' | ||
style={this.style.closeViewButton} | ||
<CloseViewButton | ||
className='style--none' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this className?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say yes, as with any other button, we don't want the default browser styling. Before we were overriding everything through .button-close
. This is more consistent now.
width: '24px', | ||
height: '24px', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like there should be a better way to do this for all the icons... But we might be stuck with it because of they're svgs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be doable with a common class. Or we could go back to compass icons which were controlled by font sizes I believe 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @streamer45! A few small requests:
- For the controls at the bottom, as per the design, the icon size should be 20x20px with a padding of 12px on all sides. Currently the icons are 24x24 with a spacing of 10px.
- Similarly for the minimize icon on the top right, the icon size should be 20x20px with 10px space on all sides.
- For the
leave call
icon button, there seems to be0.8
opacity applied on the white icon that we can get rid of. - For the threads button, wondering if we can add a border matching the background color around the notification indicator? I think it might not be straightforward though since the color is a mix of a background color and an overlay.
Thanks @abhijit-singh , will look into the above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @streamer45, looks great!
Summary
PR implements the UX polish tweaks in https://www.figma.com/file/UIfE0vN1ESsXLHZYDP5eqD/Updates-to-Control-Bar-and-Header except the call settings which I am deferring to a separate PR given it's enough work on its own and mostly decoupled from this.
Ticket Link
https://mattermost.atlassian.net/browse/MM-57263