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

Truncate long server names on login/register screen#7498

Closed
aaronraimist wants to merge 5 commits into
matrix-org:developfrom
aaronraimist:server-ellipsis
Closed

Truncate long server names on login/register screen#7498
aaronraimist wants to merge 5 commits into
matrix-org:developfrom
aaronraimist:server-ellipsis

Conversation

@aaronraimist
Copy link
Copy Markdown
Contributor

@aaronraimist aaronraimist commented Jan 10, 2022

Fixes element-hq/element-web#18452

Before:

After:


Here's what your changelog entry will look like:

🐛 Bug Fixes

Preview: https://61dc3a5abb5c3dbba7154468--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist aaronraimist requested a review from a team as a code owner January 10, 2022 12:42
@t3chguy t3chguy added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Jan 10, 2022
Copy link
Copy Markdown
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.

Great, thanks!!

Comment thread src/components/views/elements/ServerPicker.tsx
Signed-off-by: Aaron Raimist <aaron@raim.ist>
Signed-off-by: Aaron Raimist <aaron@raim.ist>
Comment on lines 72 to +77
let serverName: React.ReactNode = serverConfig.isNameResolvable ? serverConfig.hsName : serverConfig.hsUrl;
if (serverConfig.hsNameIsDifferent) {
serverName = <TextWithTooltip class="mx_Login_underlinedServerName" tooltip={serverConfig.hsUrl}>
{ serverConfig.hsName }
</TextWithTooltip>;
}
serverName = <TextWithTooltip
class={serverConfig.hsNameIsDifferent ? "mx_Login_underlinedServerName" : ""}
tooltip={serverConfig.hsUrl}>
{ serverConfig.hsName }
</TextWithTooltip>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this no longer really makes sense, L72 sets a string which doesn't get used.
And now there's always a tooltip, even if it is wholly redundant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't say wholly redundant. It shows whether http or https is being used, it shows the port being used if HS is using a nonstandard port, it shows the subdomain HS uses for client traffic, etc. Plus it is a tooltip, so it doesn't take up any extra space and only shows up on hover.

Copy link
Copy Markdown
Member

@t3chguy t3chguy Jan 10, 2022

Choose a reason for hiding this comment

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

I believe it was a design/product thing to only show it when the name doesn't match, but I agree with your evaluation

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah but that only happens because of your change, previously it'd show https://localhost:8700 inline due to the ternary statement you removed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It'd be good if this PR, labelled Truncate long server names on login/register screen did just that, rather than also tweaking the behaviour of what name is shown when

Copy link
Copy Markdown
Member

@t3chguy t3chguy Jan 10, 2022

Choose a reason for hiding this comment

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

I'm not sure what you mean. This is what it looks like on develop. No inline additional information and no tooltip with extra information.

Sounds like isNameResolvable may be set true for some reason in your case then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

image
image

^ is what I see on develop atm

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current logic to summarise is

If the HS server_name !== domain hostname: show server_name with underline & tooltip
Elif the name is resolvable (gotten via well-known) then show the server_name
Else show the URL

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yes I do see that when I click edit and enter https://localhost:8700 but it doesn't work when https://localhost:8700 is configured as the default (using default_server_config).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah that's a known behaviour for default_server_config - its always trusted to be correct and treated as if it was fetched from .well-known

Signed-off-by: Aaron Raimist <aaron@raim.ist>
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@t3chguy t3chguy self-requested a review January 10, 2022 14:24
@aaronraimist aaronraimist marked this pull request as draft January 25, 2022 03:06
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Very long homeserver address overflows on login screen

2 participants