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

Update the embed block to show no preview for smugmug embeds #12961

Merged

Conversation

brentswisher
Copy link
Contributor

Description

Fixes #11960 by adding smugmug.com to the array of domains that the "no-preview" screen is shown. Also adjusted the matching criteria to grab the base domain instead of just stripping "www." off of the host if found. This was necessary as smugmug embeds are in the format "username.smugmug.com"

How has this been tested?

Manually tested to verify that both individual photos and galleries display the no preview message using:
https://johndavis.smugmug.com/Nature/Landscape/i-NntLPZ8
https://johndavis.smugmug.com/Nature/Landscape

Screenshots

edit_post_ _gutenberg_dev_ _wordpress

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

…use fflash and causes a js error to be thrown.
@gziolo gziolo added [Block] Embed Affects the Embed Block [Type] Enhancement A suggestion for improvement. labels Dec 18, 2018
Copy link
Member

@notnownikki notnownikki left a comment

Choose a reason for hiding this comment

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

looks good to me 🚢

@ajitbohra ajitbohra merged commit b8b7bff into WordPress:master Jan 23, 2019
@jasmussen
Copy link
Contributor

❣️

@paaljoachim
Copy link
Contributor

I am doing some general testing on GB version 5.0 RC-1.
I am reacting to the word sorry.

"Sorry, this embedded content cannot be previewed in the editor."
---> It feels a lot better with "Embedded content cannot be previewed in the editor."
Or "Smugmug embedded content cannot be previewed in the editor."

@jasmussen
Copy link
Contributor

@paaljoachim Good point. Feel free to create a new ticket, you can ping me if you'd like it categorized well. I imagine such a change would be ripe territory for "Good First Ticket" labels.

const parsedUrl = parse( url );
const cannotPreview = includes( HOSTS_NO_PREVIEWS, parsedUrl.host.replace( /^www\./, '' ) );
const parsedHost = parse( url ).host.split( '.' );
const parsedHostBaseUrl = parsedHost.splice( parsedHost.length - 2, parsedHost.length - 1 ).join( '.' );
Copy link
Member

Choose a reason for hiding this comment

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

It's very non-obvious to me as a future maintainer what it is we're trying to accomplish here with this logic. It would have been very helpful to include one or more of an inline code comment, separate named and documented function, and unit test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants