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

Fix alignment and poor contrast on user pills in invite dialog #11722

Merged
merged 6 commits into from
Oct 11, 2023

Conversation

gabrc52
Copy link
Contributor

@gabrc52 gabrc52 commented Oct 8, 2023

Signed-off-by: Gabriel Rodríguez rgabriel@mit.edu

Fixes #26098

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)

Notes: Fixes invite dialog alignment and pill color contrast

Before screenshots

image

image

After screenshots

Dark theme (5.3 contrast ratio):

image

Light theme (3.5 contrast ratio):

image

Let us know if we should make a new variable (and how to name it! This would be helpful to improve the contrast for light theme without worsening for dark theme) or use an existing variable, or if you disagree with the color choice. There was no existing more specific alias for --cpd-color-blue-800: the closest was

--cpd-color-icon-info-primary: var(--cpd-color-blue-900);

but it does not seem to be enough contrast for dark theme.


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Fixes invite dialog alignment and pill color contrast (#11722). Contributed by @gabrc52.

@gabrc52 gabrc52 requested a review from a team as a code owner October 8, 2023 05:02
@gabrc52 gabrc52 requested review from dbkr and richvdh October 8, 2023 05:02
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Oct 8, 2023
@richvdh richvdh added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Oct 9, 2023
@richvdh richvdh changed the title Improve invite tile color Fix poor contrast on user pills in invite dialog Oct 9, 2023
@richvdh
Copy link
Member

richvdh commented Oct 9, 2023

This is an improvement, but it doesn't wholly fix element-hq/element-web#26098 because it doesn't address the vertical alignment of the x button.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I suspect this was introduced by #11470, which lightened the values of all the $username-variantN-color.

This fix seems vaguely plausible to me (using a random $username-variantN-color variable here seems like a bit of an abuse), but I think we probably should be using a CSS variable here.

@germain-gg this seems like your wheelhouse much more than mine; would you mind casting an eye over it?

Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Totally agree with @richvdh 's review here.

Looking at the semantic color we have defined, https://compound.element.io/?path=/docs/tokens-semantic-colors--docs
I believe --cpd-color-bg-success-subtle would be an acceptable replacement. And one that passes the accessibility requirements. Using a $username-variantN-color variable here is not fit for purpose

@gabrc52 gabrc52 changed the title Fix poor contrast on user pills in invite dialog Fix alignment and poor contrast on user pills in invite dialog Oct 9, 2023
@gabrc52
Copy link
Contributor Author

gabrc52 commented Oct 9, 2023

This is how --cpd-color-bg-success-subtle (would it be "success" because the user was successfully added to the list of people to be invited?) looks like: you almost can't tell there is a pill anymore and it would require changing the font color on light mode.

image

image

With black font:

image

This is how it would look like with --cpd-color-text-info-primary (it is equivalent to --cpd-color-blue-900, the former value of $username-variant1-color):

image

image

image

As for the alignment, I was not personally able to find what CSS changed to cause that regression, but explicitly adding vertical-align: middle; to both mx_InviteDialog_userTile_pill and mx_InviteDialog_userTile_remove does the trick. Could you confirm if it is a suitable solution, or any other insights on what could have caused it? I can confirm it is successful on Firefox on Linux, Chromium on Linux and Safari on macOS:

image

image

@germain-gg
Copy link
Contributor

germain-gg commented Oct 11, 2023

Using the background color above and changing the text colour is indeed what I'm suggesting for this case.

@gabrc52
Copy link
Contributor Author

gabrc52 commented Oct 11, 2023

Updated! This is the new before and after:

Before:

image

image

After:

image

image

@germain-gg germain-gg self-requested a review October 11, 2023 15:16
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for bearing with us and making the relevant changes.
Much appreciated!

@germain-gg germain-gg added this pull request to the merge queue Oct 11, 2023
Merged via the queue into matrix-org:develop with commit a80cf58 Oct 11, 2023
19 checks passed
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 4, 2023
Changes in [1.11.47](https://github.com/vector-im/element-web/releases/tag/v1.11.47) (2023-10-24)
=================================================================================================

## 🦖 Deprecations
 * Deprecate customisations in favour of Module API ([\#25736](element-hq/element-web#25736)). Fixes #25733.

## ✨ Features
 * element-hq/element-x-ios/issues/1824 - Convert the apple-app-site-association file to a newer format… ([\#26307](element-hq/element-web#26307)). Contributed by @stefanceriu.
 * Iterate `io.element.late_event` decoration ([\#11760](matrix-org/matrix-react-sdk#11760)). Fixes #26384.
 * Render timeline separator for late event groups ([\#11739](matrix-org/matrix-react-sdk#11739)).
 * OIDC: revoke tokens on logout ([\#11718](matrix-org/matrix-react-sdk#11718)). Fixes #25394. Contributed by @kerryarchibald.
 * Show `io.element.late_event` in MessageTimestamp when known ([\#11733](matrix-org/matrix-react-sdk#11733)).
 * Show all labs flags if developerMode enabled ([\#11746](matrix-org/matrix-react-sdk#11746)). Fixes #24571 and #8498.
 * Use Compound tooltips on MessageTimestamp to improve UX of date time discovery ([\#11732](matrix-org/matrix-react-sdk#11732)). Fixes #25913.
 * Consolidate 4s passphrase input fields and use stable IDs ([\#11743](matrix-org/matrix-react-sdk#11743)). Fixes #26228.
 * Disable upgraderoom command without developer mode enabled ([\#11744](matrix-org/matrix-react-sdk#11744)). Fixes #17620.
 * Avoid rendering app download buttons if disabled in config ([\#11741](matrix-org/matrix-react-sdk#11741)). Fixes #26309.
 * OIDC: refresh tokens ([\#11699](matrix-org/matrix-react-sdk#11699)). Fixes #25839. Contributed by @kerryarchibald.
 * OIDC: register ([\#11727](matrix-org/matrix-react-sdk#11727)). Fixes #25393. Contributed by @kerryarchibald.
 * Use stable get_login_token and remove unstable MSC3882 support ([\#11001](matrix-org/matrix-react-sdk#11001)). Contributed by @hughns.

## 🐛 Bug Fixes
 * Set max size for Element logo in search warning ([\#11779](matrix-org/matrix-react-sdk#11779)). Fixes #26408.
 * Avoid error when DMing oneself ([\#11754](matrix-org/matrix-react-sdk#11754)). Fixes #7242.
 * Fix: Message shield alignment is not right. ([\#11703](matrix-org/matrix-react-sdk#11703)). Fixes #26142. Contributed by @manancodes.
 * fix logging full event ([\#11755](matrix-org/matrix-react-sdk#11755)). Fixes #26376.
 * OIDC: use delegated auth account URL from `OidcClientStore` ([\#11723](matrix-org/matrix-react-sdk#11723)). Fixes #26305. Contributed by @kerryarchibald.
 * Fix: Members list shield alignment is not right. ([\#11700](matrix-org/matrix-react-sdk#11700)). Fixes #26261. Contributed by @manancodes.
 * Fix: <detail> HTML elements clickable area too wide. ([\#11666](matrix-org/matrix-react-sdk#11666)). Fixes #25454. Contributed by @manancodes.
 * Fix untranslated headings in the devtools dialog ([\#11734](matrix-org/matrix-react-sdk#11734)).
 * Fixes invite dialog alignment and pill color contrast ([\#11722](matrix-org/matrix-react-sdk#11722)). Contributed by @gabrc52.
 * Prevent select element in General settings overflowing in a room with very long room-id ([\#11597](matrix-org/matrix-react-sdk#11597)). Contributed by @ABHIXIT2.
 * Fix: Clicking on members pile does nothing. ([\#11657](matrix-org/matrix-react-sdk#11657)). Fixes #26164. Contributed by @manancodes.
 * Fix: Wierd shadow below room avatar in dark mode. ([\#11678](matrix-org/matrix-react-sdk#11678)). Fixes #26153. Contributed by @manancodes.
 * Fix start_sso / start_cas URLs failing to redirect to a authentication prompt ([\#11681](matrix-org/matrix-react-sdk#11681)). Contributed by @Half-Shot.

Changes in [1.11.46](https://github.com/vector-im/element-web/releases/tag/v1.11.46) (2023-10-10)
=================================================================================================

## ✨ Features
 * Use .well-known to discover a default rendezvous server for use with Sign in with QR ([\#11655](matrix-org/matrix-react-sdk#11655)). Contributed by @hughns.
 * Message layout will update according to the selected style  ([\#10170](matrix-org/matrix-react-sdk#10170)). Fixes #21782. Contributed by @manancodes.
 * Implement MSC4039: Add an MSC for a new Widget API action to upload files into the media repository ([\#11311](matrix-org/matrix-react-sdk#11311)). Contributed by @dhenneke.
 * Render space pills with square corners to match new avatar ([\#11632](matrix-org/matrix-react-sdk#11632)). Fixes #26056.
 * Linkify room topic ([\#11631](matrix-org/matrix-react-sdk#11631)). Fixes #26185.
 * Show knock rooms in the list ([\#11573](matrix-org/matrix-react-sdk#11573)). Contributed by @maheichyk.

## 🐛 Bug Fixes
 * Bump matrix-web-i18n dependency to 3.1.3 ([\#26287](element-hq/element-web#26287))
 * Fix: Avatar shrinks with long names ([\#11698](matrix-org/matrix-react-sdk#11698)). Fixes #26252. Contributed by @manancodes.
 * Update custom translations to support nested fields in structured JSON ([\#11685](matrix-org/matrix-react-sdk#11685)).
 * Fix: Edited message remove button is hard to reach. ([\#11674](matrix-org/matrix-react-sdk#11674)). Fixes #24917. Contributed by @manancodes.
 * Fix: Theme selector radio button not aligned in center with the text ([\#11676](matrix-org/matrix-react-sdk#11676)). Fixes #25460. Contributed by @manancodes.
 * Fix: Unread notification dot aligned ([\#11658](matrix-org/matrix-react-sdk#11658)). Fixes #25285. Contributed by @manancodes.
 * Fix: sync intentional mentions push rules with legacy rules ([\#11667](matrix-org/matrix-react-sdk#11667)). Fixes #26227. Contributed by @kerryarchibald.
 * Revert "Fix regression around FacePile with overflow (#11527)" ([\#11634](matrix-org/matrix-react-sdk#11634)). Fixes #26209.
 * Fix: Alignment Fixed ([\#11648](matrix-org/matrix-react-sdk#11648)). Fixes #26169. Contributed by @manancodes.
 * Fix: onFinished added which closes the menu ([\#11647](matrix-org/matrix-react-sdk#11647)). Fixes #25556. Contributed by @manancodes.
 * Don't start key backups when opening settings ([\#11640](matrix-org/matrix-react-sdk#11640)).
 * Fix add to space avatar text centering ([\#11643](matrix-org/matrix-react-sdk#11643)). Fixes #26154.
 * fix avatar styling in lightbox ([\#11641](matrix-org/matrix-react-sdk#11641)). Fixes #26196.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invite dialog pills have poor styling
3 participants