Add truncation for image URLs #21242

isabellachen · 2020-03-29T16:43:20Z

Description

Fixes #21100
Truncate displayed media urls as outlined in Issue 21100

How has this been tested?

Additional tests were added to the tests for filterURLForDisplay.
The testing environment is the default one with jest. I ran npm run test-unit

Does this change affect other areas of code?

If a link links directly to an image, the link displayed will be truncated

Display of non-image links are not affected

Types of changes

This change is an update/ enchancement

Checklist:

[ x ] My code is tested.
[ x ] My code follows the WordPress code style.
[ x ] My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

aduth

Hi @isabellachen, thanks for your first contribution!

To be honest, while I kind of understand what #2110 is trying to describe as problematic, it's still not entirely clear to me what it is that is special about media in particular that we would want to apply this treatment. Is there some prior art we can reference for this being a convention in other applications? Is it something where we don't need to target "media" files specifically, but rather any URL which has a file extension, if the idea is that it may be important to put focus on the filename + file extension of the URL?

aduth · 2020-03-31T17:20:14Z

packages/url/src/filter-url-for-display.js

@@ -19,5 +19,26 @@ export function filterURLForDisplay( url ) {
 		return filteredURL.replace( '/', '' );
 	}

+	// Prettify image urls
+	const mediaRegexp = /([\w|:])*\.(?:jpg|jpeg|gif|png|svg)/g;


All tests pass if I change this to...

Suggested change

const mediaRegexp = /([\w|:])*\.(?:jpg|jpeg|gif|png|svg)/g;

const mediaRegexp = /\.(?:jpg|jpeg|gif|png|svg)/g;

I'd wonder then:

Can we remove it?

Or, are we missing a test case to protect against whatever its purpose for existing is?

It looks safe to check only for extensions but that would include all kinds of media extensions (audio and video too)

aduth · 2020-03-31T17:20:28Z

packages/url/src/filter-url-for-display.js

@@ -19,5 +19,26 @@ export function filterURLForDisplay( url ) {
 		return filteredURL.replace( '/', '' );
 	}

+	// Prettify image urls
+	const mediaRegexp = /([\w|:])*\.(?:jpg|jpeg|gif|png|svg)/g;


Do we need to be matching this globally? Are we expecting multiple matches?

Suggested change

const mediaRegexp = /([\w|:])*\.(?:jpg|jpeg|gif|png|svg)/g;

const mediaRegexp = /([\w|:])*\.(?:jpg|jpeg|gif|png|svg)/;

aduth · 2020-03-31T17:25:17Z

packages/url/src/filter-url-for-display.js

+	const colon = ':';
+	const period = '.';
+	const query = '?';


Are these serving much purpose? A few points I'd make:

In the context of their relevance to this function, they're only relevant within the if condition. While it's not much of a problem in this case, in other instances if these assignments were non-trivial function executions, it could cause some wasted resources if that if condition is not matched. This is quite similar to what we have enforced in the custom no-unused-vars-before-return ESLint rule.

But in any case, to me they appear to fall under the definition of a constant, and as such, would be best to rename and move to the top-most scope (reference documentation).

Even still, I'm not clear on what their purpose is. And if it's related to matching ports and query strings, it would be good if we could avoid using string manipulation to work with these values, if we can instead rely on using a URL constructor and one of the properties of its instance (pathname, etc).

Related: URL: Implement custom getQueryArgs, buildQueryString #20693

Indeed using

let url = new URL('https://developer.mozilla.org/example.mov');

will return a nice object with all the elements decomposed like this:

hash: "" host: "developer.mozilla.org" hostname: "developer.mozilla.org" href: "https://developer.mozilla.org/example.mov" origin: "https://developer.mozilla.org" password: "" pathname: "/example.mov" port: "" protocol: "https:" search: "" searchParams: URLSearchParams { } username: ""

so less need to split/join strings.

aduth · 2020-03-31T17:26:11Z

packages/url/src/filter-url-for-display.js

+		if ( tokens[ 0 ].includes( period ) ) {
+			tokens = tokens[ 0 ].split( period );
+		}
+		const prettifiedImageURL = tokens[ 0 ] + '...' + imageToken;


Seems an ellipsis character … would be most appropriate here.

Related:

https://en.wikipedia.org/wiki/Ellipsis

gutenberg/.eslintrc.js

Lines 88 to 92 in 7e964a8

{

selector:

'CallExpression[callee.name=/^(__|_x|_n|_nx)$/] Literal[value=/\\.{3}/]',

message: 'Use ellipsis character (…) in place of three dots',

},

isabellachen · 2020-03-31T17:56:20Z

@aduth Thanks for the review. I take your point about targeting any URL with a file extension, maybe I should consider this?

As for your question about this being a convention... I don't think I've seen the proposed solution elsewhere. Also, if the file name is even a bit long e.g. cape-verde-santo-antao-1234.jpg you end up with cape-verde... so you don't get to see the extension.

Maybe @draganescu can provide an opinion on whether to target all URLs with file extensions and if its more important to preserve the start or the end.

I will look at implementing your feedback over the coming weekend :)

draganescu · 2020-04-02T09:03:34Z

Howdy @isabellachen and thanks for this work!
My 2 cents on the issue are as follows:

link urls, the ones assigned to bits of text in paragraphs or in menu links have less important endings as the geenral domain name (if external) or link title (if internal) are enough indicators for users that they've set the right thing.
for media items, it points to the resource and so the filename might be important. As to when exactly, the only situation I can think of when you'd need that final part of the URL to be visible is when you have similar media items and you want to be sure you have inserted the right one (final_01.jpg vs final_final_01.png). In all other situations the media file itself is a bigger, easier sign that you have the correct thing.

@noisysocks who had the original comment for this issue might chime in with more detail, if any.

draganescu

I believe, if the need looks justified that perhaps the MediaReplaceFlow component should implement this as it seems something specific to that workflow. cc: @noisysocks

draganescu · 2020-04-02T09:06:44Z

packages/url/src/filter-url-for-display.js

@@ -19,5 +19,26 @@ export function filterURLForDisplay( url ) {
 		return filteredURL.replace( '/', '' );
 	}

+	// Prettify image urls
+	const mediaRegexp = /([\w|:])*\.(?:jpg|jpeg|gif|png|svg)/g;


It looks safe to check only for extensions but that would include all kinds of media extensions (audio and video too)

draganescu · 2020-04-02T09:14:48Z

packages/url/src/filter-url-for-display.js

+	const colon = ':';
+	const period = '.';
+	const query = '?';


Indeed using

let url = new URL('https://developer.mozilla.org/example.mov');

will return a nice object with all the elements decomposed like this:

hash: "" host: "developer.mozilla.org" hostname: "developer.mozilla.org" href: "https://developer.mozilla.org/example.mov" origin: "https://developer.mozilla.org" password: "" pathname: "/example.mov" port: "" protocol: "https:" search: "" searchParams: URLSearchParams { } username: ""

so less need to split/join strings.

isabellachen · 2020-04-02T10:11:54Z

@draganescu Thanks for your feedback! Very handy, this use URL(). I also did think the code might have to be moved out of this file (filter-url-for-display.js) to the MediaReplaceFlow component. The MediaReplaceFlow component is using another component to display links though, so I'll probably have to add a prop on it so it knows to display media links differently. Waiting on @noisysocks for further input, otherwise will take this approach :)

noisysocks · 2020-04-03T03:14:37Z

I've no opinion about regular link URLs. In the media flow, it is helpful to see the end of the URL because the beginning will nearly always be my website domain. It is the path component that helps me tell which image I selected.

Regarding prior art, Finder in macOS comes to mind:

aduth · 2020-04-03T14:29:19Z

I've no opinion about regular link URLs. In the media flow, it is helpful to see the end of the URL because the beginning will nearly always be my website domain. It is the path component that helps me tell which image I selected.

Regarding prior art, Finder in macOS comes to mind:

The prior art is helpful, for sure 👍

I do wonder if this is more of a behavior of how to treat URLs generally when confined to a restricted width, even further away from the idea of this being specific to media.

There's some relevant discussion on this topic:

https://stackoverflow.com/questions/206899/whats-the-best-way-to-truncate-a-url-so-that-it-fits-within-a-layout
- end of the link as the most value in terms of giving context:
- display the most critical components of the URL. This is the file being requested and the domain of the request are what I consider critical, the intermediate path and the query string are things I consider non-critical.
- Get rid of the middle, no one needs to know the directory structure of a link. The domain is important and the actual file is important.
- I'd expect to see at least the server.
https://ux.stackexchange.com/questions/40326/best-way-to-handle-long-urls
- I would recommend putting the ellipses in the middle of the shortened string rather than at the end. It is commonly the last portion of the URL that distinguishes it from others, so by putting the ellipses in the middle you are not truncating the useful part of the URL.

I think it becomes trickier when you try to consider which URLs should be truncated. If we have the available space, it probably shouldn't be necessary to truncate? How would it be configured?

One approach is that filterURLForDisplay could receive a maxLength option, and the responsibility of the implementation is to make the best compromise of fitting the URL within those constraints (within reason).

There's a separate problem of "How does the media replace dropdown menu know what to use for maxLength?" I don't really think it's a problem for the @wordpress/url module to solve. A simplest solution could be to use some arbitrary value that happens to work, if we know that the width isn't likely to change. Otherwise, we'd probably need some complex machinery of "find the number of characters of a particular font of a particular font size that can fit within the given measurement of a DOM element".

Mamaduka · 2021-09-07T12:30:08Z

Hello, @isabellachen

Thanks so much for working on this. It looks like others were interested in resolving this issue because there's PR that got merged and addressed the same area of work. I’m going to close this out as a result.

isabellachen added 2 commits March 29, 2020 16:47

Add truncation for image URLs

b9f8379

Add code for more complex cases

769fa49

isabellachen requested review from aduth and talldan as code owners March 29, 2020 16:43

aduth requested changes Mar 31, 2020

View reviewed changes

aduth added [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Feature] Media Anything that impacts the experience of managing media [Package] Url /packages/url labels Mar 31, 2020

aduth requested a review from draganescu March 31, 2020 17:29

aduth added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 31, 2020

draganescu reviewed Apr 2, 2020

View reviewed changes

Base automatically changed from master to trunk March 1, 2021 15:43

Mamaduka closed this Sep 7, 2021

Mamaduka mentioned this pull request Jun 30, 2022

Rewrite URL (Permalink) panel as a popover #42033

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add truncation for image URLs #21242

Add truncation for image URLs #21242

isabellachen commented Mar 29, 2020 •

edited by draganescu

Loading

aduth left a comment •

edited

Loading

aduth Mar 31, 2020

draganescu Apr 2, 2020

aduth Mar 31, 2020

aduth Mar 31, 2020

draganescu Apr 2, 2020

aduth Mar 31, 2020

isabellachen commented Mar 31, 2020

draganescu commented Apr 2, 2020

draganescu left a comment

draganescu Apr 2, 2020

draganescu Apr 2, 2020

isabellachen commented Apr 2, 2020

noisysocks commented Apr 3, 2020 •

edited

Loading

aduth commented Apr 3, 2020

Mamaduka commented Sep 7, 2021

	const mediaRegexp = /([\w\|:])*\.(?:jpg\|jpeg\|gif\|png\|svg)/g;
	const mediaRegexp = /\.(?:jpg\|jpeg\|gif\|png\|svg)/g;

	{
	selector:
	'CallExpression[callee.name=/^(__\|_x\|_n\|_nx)$/] Literal[value=/\\.{3}/]',
	message: 'Use ellipsis character (…) in place of three dots',
	},

Add truncation for image URLs #21242

Add truncation for image URLs #21242

Conversation

isabellachen commented Mar 29, 2020 • edited by draganescu Loading

Description

How has this been tested?

Does this change affect other areas of code?

Types of changes

Checklist:

aduth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabellachen commented Mar 31, 2020

draganescu commented Apr 2, 2020

draganescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabellachen commented Apr 2, 2020

noisysocks commented Apr 3, 2020 • edited Loading

aduth commented Apr 3, 2020

Mamaduka commented Sep 7, 2021

isabellachen commented Mar 29, 2020 •

edited by draganescu

Loading

aduth left a comment •

edited

Loading

noisysocks commented Apr 3, 2020 •

edited

Loading