Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(navigation): improve url presentation in redirect warning message #13474

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Dec 9, 2021

before

"The page may not be loading as expected because your test URL (chromestatus.com/features) was redirected to chromestatus.com/features. Try testing the second URL directly."

after

"The page may not be loading as expected because your test URL ( chromestatus.com/features ) was redirected to ( chromestatus.com/features )—try testing the second URL directly."

Padding the url with spaces on either side improves url detection. for example, see how the url is wrong for the old format here:
image

Closes #15536

@connorjclark connorjclark requested a review from a team as a code owner December 9, 2021 00:50
@connorjclark connorjclark requested review from adamraine and removed request for a team December 9, 2021 00:50
@brendankenny
Copy link
Member

Maybe need to work this more? The spaces look pretty janky and this warning is quite prominent in the report (and honestly URL detection in someone's terminal setup seems low priority :P)

@brendankenny
Copy link
Member

Embracing the em-dash? That wouldn't solve the period though. nbsp, but that likely wouldn't survive the translation process (though extra spaces probably won't either)

@adamraine
Copy link
Member

Honestly I would prefer a larger overhaul to the warning message. It get's pretty hard to read for long URLs:

Screen Shot 2021-12-09 at 11 35 58 AM

I think it would be better to turn these into links using the markdown renderer like:

The page may not have loaded correctly because the [requested URL](https://google.com) was different from the [final URL](https://www.google.com)
@paulirish
Copy link
Member

Honestly I would prefer a larger overhaul to the warning message. It get's pretty hard to read for long URLs:

we can use Util.getURLDisplayName plus a title attribute with the full thing

@connorjclark
Copy link
Collaborator Author

maybe:

The page may not be loaded bc...... :
- https://www.example.com
- https://www.example.com?addasdsad 

@exterkamp did this in PSI...

image

@connorjclark
Copy link
Collaborator Author

connorjclark commented Dec 13, 2022

we can use Util.getURLDisplayName plus a title attribute with the full thing

Using Util.getURLDisplayName(url, {preserveHost: true, preserveQuery: true}) shortens it, but visually the result of that function is likely to be equivalent for both urls involved in the redirect. For example, the common http -> https case would always look the same:

for http://www.paulirish.com/ -> https://www.paulirish.com/

paulirish.com/
paulirish.com/

maybe:

The page may not be loaded bc...... :
- https://www.example.com
- https://www.example.com?addasdsad 

We'd have to support a list - in our markdown (or - simpler - encode something that means <br>. Not sure if raw \n or <br> could be considered standard-enough markdown for this to be reasonable approach), but doable:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants