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

Tweak appearance of invite reason #5847

Merged
merged 4 commits into from
Apr 12, 2021
Merged

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Apr 12, 2021

This adjusts the display of invite reasons to match design feedback.

image

image

Fixes element-hq/element-web#16869

This adjusts the display of invite reasons to match design feedback.

Fixes element-hq/element-web#16869
@jryans jryans requested review from niquewoodhouse and a team April 12, 2021 11:32
@t3chguy
Copy link
Member

t3chguy commented Apr 12, 2021

The contrast on View message is rough, I struggle to read it and don't have any sight issues

@jryans
Copy link
Collaborator Author

jryans commented Apr 12, 2021

Updated to remove blur effect after discussing with @niquewoodhouse.

Copy link
Contributor

@niquewoodhouse niquewoodhouse left a comment

Choose a reason for hiding this comment

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

Works for me. Can iterate after feedback/shipping, eg add avatar/blur/etc.

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 other than some naming nits

res/css/views/elements/_InviteReason.scss Outdated Show resolved Hide resolved
src/components/views/elements/InviteReason.tsx Outdated Show resolved Hide resolved
Comment on lines 28 to 31
top: 0;
bottom: 0;
left: 0;
right: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if you already know that, but I've only discovered this recently. There's now a shorthand for all those properties, inset

Suggested change
top: 0;
bottom: 0;
left: 0;
right: 0;
inset: 0;

I believe PostCSS should do its job and replace that in the final bundle

Copy link
Member

Choose a reason for hiding this comment

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

Though empirically PostCSS does not translate it

image

Looks like we'd need to install https://www.npmjs.com/package/postcss-inset if we wanted that

Given lack of Safari support https://developer.mozilla.org/en-US/docs/Web/CSS/inset we should not use it without postcss support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to inset.

Copy link
Member

Choose a reason for hiding this comment

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

@jryans probably best to revert that to not risk it breaking under Safari

@jryans jryans merged commit 098a871 into develop Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design feedback on showing invite reasons
4 participants