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.

Sign up for GitHub

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

Add truncation for image URLs #21242

Closed
wants to merge 2 commits into from
Closed

Add truncation for image URLs #21242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9f8379
Add truncation for image URLs
isabellachen Mar 29, 2020
769fa49
Add code for more complex cases
isabellachen Mar 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/url/src/filter-url-for-display.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,26 @@ export function filterURLForDisplay( url ) {
return filteredURL.replace( '/', '' );
}

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

Choose a reason for hiding this comment

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

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:

  1. Can we remove it?
  2. Or, are we missing a test case to protect against whatever its purpose for existing is?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)/;

const colon = ':';
const period = '.';
const query = '?';
Comment on lines +24 to +26
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if ( filteredURL.match( mediaRegexp ) ) {
let tokens = filteredURL.split( '/' );
let imageToken = tokens[ tokens.length - 1 ];
if ( imageToken.includes( query ) ) {
imageToken = imageToken.split( query )[ 0 ];
}
if ( tokens[ 0 ].includes( colon ) ) {
tokens = tokens[ 0 ].split( colon );
}
if ( tokens[ 0 ].includes( period ) ) {
tokens = tokens[ 0 ].split( period );
}
const prettifiedImageURL = tokens[ 0 ] + '...' + imageToken;
Copy link
Member

Choose a reason for hiding this comment

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

Seems an ellipsis character … would be most appropriate here.

Related:

return prettifiedImageURL;
}

return filteredURL;
}
36 changes: 36 additions & 0 deletions packages/url/src/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,42 @@ describe( 'filterURLForDisplay', () => {
const url = filterURLForDisplay( 'http://www.wordpress.org/something' );
expect( url ).toBe( 'wordpress.org/something' );
} );
it( 'should truncate image URLs in the middle', () => {
const url = filterURLForDisplay( 'https://www.example.com/hello.jpg' );
expect( url ).toBe( 'example...hello.jpg' );
} );
it( 'should truncate image URLs hosted on localhost', () => {
const url = filterURLForDisplay(
'http://localhost:8888/wp-content/uploads/2020/03/hello.jpg'
);
expect( url ).toBe( 'localhost...hello.jpg' );
} );
it( 'should truncate complex image URLs', () => {
const url = filterURLForDisplay(
'https://gallery.google.co.uk:80/hello.jpg'
);
expect( url ).toBe( 'gallery...hello.jpg' );
} );
it( 'should truncate image URLs from CDN', () => {
const url = filterURLForDisplay(
'https://i1.wp.com/example.com/wp-content/uploads/2020/03/prague.jpg'
);
expect( url ).toBe( 'i1...prague.jpg' );
} );
it( 'should truncate query from image URL', () => {
const url = filterURLForDisplay(
'https://www.example.com/prague.jpg?w=1200&ssl=1'
);
expect( url ).toBe( 'example...prague.jpg' );
} );
it( 'should not truncate images URLs with no extension', () => {
const url = filterURLForDisplay(
'https://images.unsplash.com/photo-123-auto=format&fit=crop&w=1650&q=80'
);
expect( url ).toBe(
'images.unsplash.com/photo-123-auto=format&fit=crop&w=1650&q=80'
);
} );
} );

describe( 'cleanForSlug', () => {
Expand Down