Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix outstanding UX issues with replies/mentions/keyword notifs#28270

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
t3chguy merged 10 commits intoelement-hq:developfromtaffyko:mention-text-and-pills-ux
Jan 22, 2025

Conversation

taffyko
Copy link
Contributor

@taffykotaffyko commentedOct 22, 2024•
edited by t3chguy
Loading

This is a continuation ofPR #85 from the now-incorporated matrix-react-sdk, see the discussion there.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updatedpublic/exported symbols have accurateTSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing theContributor License Agreement (CLA)

Purpose

For the "Modern" layout, a bright stripe has been added to the left edge of the highlighted-message background to make mentions/notifications more visible, in the spirit ofelement-hq/element-meta#1476.
EDIT: This addition has been dropped from this PR, as it needs additional work to adhere to the Compound design system guidelines — I believe it would be best for me to address it in a follow-up PR instead.
For now, disregard the highlighted left edge that appears in the "After" screenshots below.

BeforeAfter
Pasted image 20240922225036Pasted image 20240922225435
Pasted image 20240922225030Pasted image 20240922225105

Fixeselement-hq/element-meta#744
Fixes#8821

K4LCIFER, Avamander, ThowZzy, Croydon, and Sereza7 reacted with rocket emoji
I foresee this change being made across the codebase shortlyand want to proactively prevent my PR from falling behind
@taffykotaffyko requested a review froma team as acode ownerOctober 22, 2024 21:46
@CLAassistant
Copy link

CLAassistant commentedOct 22, 2024•
edited
Loading

CLA assistant check
All committers have signed the CLA.

It is clear that it would be best for me to addressthis piece in a separate PR.
@richvdh
Copy link
Member

@dbkr's comparison with EX:

  • username pills are green
  • the rest of the message isnot highlighted
  • keywords aren't currently highlighted at all

@richvdh
Copy link
Member

@t3chguy will take this to the design team for guidance

@dbkr
Copy link
Member

dbkr commentedJan 9, 2025

For reference, this is what mention and keywords look like in EX iOS:
IMG_13AE0C952934-1

@t3chguy
Copy link
Member

I have requested input from @element-hq/design but did not get anywhere.

@t3chguy
Copy link
Member

Hey@taffyko - I managed to get some review from the @element-hq/design team. They are happy to land this but would like the font colour for pills to also be updated to--cpd-color-text-on-solid-primary at the same time. Are you able to get this done? Thanks

@taffyko
Copy link
ContributorAuthor

Hey@taffyko - I managed to get some review from the @element-hq/design team. They are happy to land this but would like the font colour for pills to also be updated to--cpd-color-text-on-solid-primary at the same time. Are you able to get this done? Thanks

On it!

@taffyko
Copy link
ContributorAuthor

@t3chguy
I've updated with latestdevelop and tested.
Pills of all types are are already using--cpd-color-text-on-solid-primary for their text color (dark mode and light mode alike), I believe we're all good here!
image
image

Copy link

@gaelledelgaelledel left a comment

Choose a reason for hiding this comment

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

Thanks very much@taffyko

taffyko reacted with heart emoji
Copy link
Member

@t3chguyt3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane to me, thanks!

Comment on lines +268 to +272
private regExpForKeywordPattern(pattern: string): RegExp {
// Reflects the push notification pattern-matching implementation at
// https://github.com/matrix-org/matrix-js-sdk/blob/dbd7d26968b94700827bac525c39afff2c198e61/src/pushprocessor.ts#L570
return new RegExp("(^|\\W)(" + globToRegexp(pattern) + ")(\\W|$)", "i");
}
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to reuse the cache in the js-sdk for these regexes but not going to block this change on that

@t3chguyt3chguy added this pull request to themerge queueJan 22, 2025
Merged via the queue intoelement-hq:develop with commit68c03dbJan 22, 2025
28 checks passed
@clokep
Copy link

Very excited to see this! Thanks for taking the time to do it!

taffyko, ThowZzy, and Croydon reacted with heart emojit3chguy and ThowZzy reacted with rocket emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@t3chguyt3chguyt3chguy approved these changes

@gaelledelgaelledelgaelledel approved these changes

Assignees
No one assigned
Labels
T-EnhancementZ-Community-PRIssue is solved by a community member's PR
Projects
None yet
Milestone
No milestone
7 participants
@taffyko@CLAassistant@richvdh@dbkr@t3chguy@clokep@gaelledel
[8]ページ先頭
©2009-2025 Movatter.jp