Format mxids when disambiguation needed

[SimonBrandner]

Description

Fixes #16897
Requires matrix-org/matrix-js-sdk#1730

Showcase

Dark

No display name:

Disambiguated + flair:

Light

Disambiguated + flair:

[t3chguy]

Does this always disambiguate rather than only when needed?

[SimonBrandner]

Yep, I misunderstood the issue... I'll rework this But at least there is something to play with :D Should be reworked now.

[t3chguy]

it looks sleek

[t3chguy]

Could you please add screenshots exercising Flair and a no displayname user please

[t3chguy]

Code looks fine

Could benefit from a TS conversion if you fancy it

CSS unsupported in Safari is a blocker, unless PostCSS does some magic on it, let me know if it does.

[t3chguy]

Code LGTM

[niquewoodhouse]

Really like this change!

Tried to review but couldn't find a room with disambiguation happening 🤔 - all the rooms I've seen it in before just didn't have it, am I missing something?

Judging from the screenshot (so its a bit harder to give solid feedback)

• Colour: could you please confirm the colour of the disambiguation? Is it the same as the timestamp?

• The [ ] has made the baseline jump so the username and display name characters look like they have different baselines, which makes it harder to read. For example, on Twitter the @ and the name have the same baseline, making it easier to scan across and understand the order. And on Discord, the timestamp has the same baseline as the username. Every small misalignment in the timeline can distract the eye and make it slightly harder to read it. This kind of leads me into the next point...

• I don't personally get the [ ]. People can create accounts and use any Matrix client without knowing what Matrix is, let alone seeing/remembering the matrix logo so the connection feels tenuous, confusing and, in the product of Element, inconsistent. For example, I can't see it on the user profile panel, but I can see it here...why? What does it all mean? 🤔

If people want to use the [ ] in the product, I'd personally suggest it's a separate issue and looks at it consistently across the product. I'd still personally advocate for it not being there but at least the consistency point would be irrelevant. Maybe I'm missing something really obvious because I don't necessarily know Matrix as well as others but it doesn't feel to me like the right thing to arbitrarily do here.

• What happens if I click on the [@jewjijwi:homeserver.com]? Is it the same as if I click on the name, or the avatar? Or nothing?

Thanks

[turt2live]

(the easiest way to cause disambiguation is to set up a GitHub notification bot or RSS bot through the integration manager)

[ShadowJonathan]

If people want to use the [ ] in the product, I'd personally suggest it's a separate issue and looks at it consistently across the product. I'd still personally advocate for it not being there but at least the consistency point would be irrelevant. Maybe I'm missing something really obvious because I don't necessarily know Matrix as well as others but it doesn't feel to me like the right thing to arbitrarily do here.

This indeed does sound like a Product issue, I think that for now it'll be good to keep consistency and keep () parentheses around the disambiguation tags. I'll create another issue in element-web (for Product) that asks this question.

[turt2live]

I'm not convinced it needs to be deferred work here... we can just fix it in this PR, surely?

[SimonBrandner]

I'm not convinced it needs to be deferred work here... we can just fix it in this PR, surely?

Yeah, definitely

[SimonBrandner]

Oops :D

[ShadowJonathan]

I'm not convinced it needs to be deferred work here... we can just fix it in this PR, surely?

I meant; Change it back to () here, and i'll make an issue in element-web discussing if using [] instead is okay.

Edit: Actually, after talking with @SimonBrandner i think changing this to no parentheses would be good? The original () wrapping was to ensure that it was disambiguated in-band in the "displayname" of the user, and was more or less an artifact, i think unwrapping it now that it is "out of band" of the displayname is a better idea instead.

[turt2live]

I'd rather we discuss it here given it's entirely within the context of this change. Also from the looks of it, the suggestion seems to be heading towards no brackets, not square vs round.

[ShadowJonathan]

What happens if I click on the [@jewjijwi:homeserver.com]? Is it the same as if I click on the name, or the avatar? Or nothing?

The name, AFAIK, so i'll insert a mention into the current composer.

Tried to review but couldn't find a room with disambiguation happening 🤔 - all the rooms I've seen it in before just didn't have it, am I missing something?

I'm in the #element-dev room with an alt, so you can scroll up (or ping me to make some messages) and observe the changes.

[t3chguy]

Could you please add screenshots exercising Flair and a no displayname user please

[SimonBrandner]

Could you please add screenshots exercising Flair and a no displayname user please

It's on my list of things to do - sorry for the delay

[t3chguy]

(I was noting it as a request so it doesn't get lost and someone merge without them is all, no rush)

[SimonBrandner]

@t3chguy @SimonBrandner from the screenshots it looks like we're using some different typography for the mxid.

Yep, that was the point. Users had issues understanding what the mxid meant, so we were hoping that if we used a different font and color, it would be more understandable. You can read the original issue that this PR fixes for more detail: element-hq/element-web#16897

[ShadowJonathan]

The typography change was my decision partially also, I thought that a monospaced font would be better representative of a "user ID", something closer to "technical", as (imo) monospace == technical. It also contributes to the "out of band" feeling i was going for, a different typography (and size) helps convey a difference in the bits of information the users would be reading, there and then.

If it's going to be an issue to grab from the existing typography templates, i'll personally be okay with changing it to a "known" font right now, and think about which monospace font to add to the element design templates later, and then to apply to this.

[SimonBrandner]

The preview is probably broken since I didn't update the js-sdk branch. I'll do that when I get home - 6 hours from now

[ShadowJonathan]

New js-sdk PR: matrix-org/matrix-js-sdk#1730

[SimonBrandner]

@nadonomy, the new preview should be working just fine. Sorry for the delay on this

[nadonomy]

@SimonBrandner thanks! And np on the delay/build.

So, after testing, I'm more resolute on the use of distinct typography in this use case being a misstep. I understand the rationale and goal of wanting to use a more technical looking font to communicate a more technical piece of information. On reflection though, I think it serves anti-goals:

1. Not advantageously normalising Matrix IDs.
2. Inconsistency. I can't rationalise using a distinct font here but not elsewhere. It would only breed confusion in a number of contexts.
3. A lack of legibility.

Here's the PR as it stands:

And here's a visual iteration using devtools:

To arrive at this I removed the font-family: monospace; declaration and bumped the font size to font-size: 1.1rem; to aid legibility. @SimonBrandner would you be ok to match the same visual iteration to approve/pass design review?

And thanks for all of your effort on this so far. :)

[ShadowJonathan]

I'm not exactly sure if the brightness value of that last screenshot would be exactly what I was aiming for (a de-emphasised piece of information that can - if needed - be glazed over), the brightness there is as high as the actual username, and I think it adds some subtle confusion.

Wouldn't it work to lower it, to either some brightness like the state-change motif above ("[...] joined the room"), or somewhere in between? Sorry to add this quick design suggestion.

Edit: to be clear, I agree with the reasoning of the changes made, I just think the change doesn't address the original thing I wanted it to do; de-couple the disambiguated MXID from the displayname, and make it "different" enough. I think that previously the font took the job of that, but right now, sans color, it looks almost the same to me, except a slightly misaligned font size, that's my feedback.

[SimonBrandner]

@nadonomy, thanks for the review! I've updated the styling based on your suggestions

[ShadowJonathan]

Poking at devtools a little and picking #81848a, i get the following;

@nadonomy what do you think about this?

[SimonBrandner]

Poking at devtools a little and picking #81848a, i get the following;

@nadonomy what do you think about this?

Personally, I like this one better

[nadonomy]

Gotcha. Deferring it in the hierarchy is a great idea/suggestion. However, we shouldn't insert new magic values into the codebase. It creates debt (at one point we had >100 different shades of grey) and we're working to unify all the styles in the app.

It'd be better to use an existing style, to ease unifying them.

From inspecting, can we match what we do for the join/leave styles pls? It just inherits body colour (so in light: #2e2f32, dark: #FFFFFF) with opacity: 0.5 and visually will look like the screenshots posted.

[ShadowJonathan]

It'd be better to use an existing style, to ease unifying them.

Of course, that's what I meant to intend, though I didn't write that down explicitly. I saw somewhere else there's a color style Figma document, and efforts to unify the colors to that document, I eyeballed the color because I wanted to convey the meaning, I'll leave it up to you and Simon to find the right one, but yes, I meant to "defer it in the hierarchy", I just couldn't find those words.

Here are the corresponding screenshots with dark and light:

[SimonBrandner]

@nadonomy, I've updated the styling, so now it should look like join/leave (and other .mx_TextualEvent):

[nadonomy]

Great! Thanks all. :)

On these screenshots the font weight looks like it might be a touch too heavy, but hard to tell without using it in practise across multiple screens, resolutions, aliasing settings etc.

Approving, but may tweak the visuals after more testing!

[SimonBrandner]

Thanks for all the feedback

Approving, but may tweak the visuals after more testing!

Sure, let me know if you have any further suggestions after using this in the wild

[SimonBrandner]

(re-requested a code review as I've made a few changes)

[t3chguy]

LGTM otherwise