Blocks: Avoid separate host matching constant for embeds #13755
Conversation
aduth
added
[Type] Code Quality
aduth
requested review from
ajitbohra,
gziolo,
jorgefilipecosta,
noisysocks,
Soean,
talldan and
youknowriad
as code owners
| className, | ||
| icon, | ||
| label, | ||
| } ) { | ||
| const { scripts } = preview; | ||
|
|
||
| const html = 'photo' === type ? getPhotoHtml( preview ) : preview.html; | ||
| const parsedHost = parse( url ).host.split( '.' ); | ||
| const parsedHostBaseUrl = parsedHost.splice( parsedHost.length - 2, parsedHost.length - 1 ).join( '.' ); |
There was a problem hiding this comment.
My original hope was to remove this logic, as a concern I noted at #12961 in trying to undertake the task described at #13386. I wonder if there's any reason we ought not use the title property of the embed in place (or at least as preferred to) the parsed host name of the URL, e.g. "Embedded content from Facebook" rather than "Embedded content from facebook.com".
(Note: Regardless, I'd consider this a separate task outside the scope of the pull request)
There was a problem hiding this comment.
I think that the title would be good, then the url parsing logic can be removed entirely. If you look at #13691 the title is actually what was preferred.
I do want to make sure that #13691 still gets addressed, is that something I can add to your branch here, or would it be better to wait till this is in master and then rebase #13715 over the top of it?
Still learning the ropes here and don't want to make things difficult for reviewers.
There was a problem hiding this comment.
I do want to make sure that #13691 still gets addressed, is that something I can add to your branch here, or would it be better to wait till this is in master and then rebase #13715 over the top of it?
I'd prefer to keep each pull request independent, hence why I'd considered any other changes to be made following this one. So, to your point, rebasing when and if necessary would be preferred. Looking at your changes from #13715 and considering I'm not yet making any changes to parsedHostBaseUrl, I suspect you shouldn't encounter any merge conflicts though.
|
Insomnia induced mini-review: yes, this totally a step in the right direction, should never have been based on domains. Will look deeper into the code on Monday. Thank you for this! |
gziolo
added
the
Good First Review
|
What is the status of this PR? |
|
@ZebulanStanphill It's been some time since I looked at this, but as far as I can recall, it was simply waiting for a review. By now, it'll need to be rebased though. |
|
@aduth do you remember if there was anything blocking this PR? |
|
I don't believe so. I know that the embed pattern for SmugMug was updated separately in #21744. I can see about giving this one a refresh. |
aduth
force-pushed
the
remove/embed-host-matching
branch
from
ea10e03
to
93f6044
Compare
|
Size Change: +3 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
This pull request seeks to refactor how we consider non-previewable hosts to avoid the need to maintain a list of host names which are separate from the settings definitions of the blocks on which they act. The changes here assign
previewableas a property of the Facebook and SmugMug embed blocks. This also helps reduce redundancy between the host names and pattern matching of a block, since the former had forced the implementation of hostname normalization which otherwise wouldn't be necessary when relying on the existing patterns matching. It is also consistent with other embed-specific behaviors (e.g.responsive) being assigned incore-embeds.js.An interesting consequence of the fact that these were maintained separately is that the pattern for SmugMug URLs was inaccurate since it didn't capture subdomains (and likewise did not match the equivalent pattern defined in PHP). Thus, it was not being transformed to the "SmugMug" embed variant. It became obvious there was an issue when testing a SmugMug URL had initially regressed the issue described in #11960. This was then fixed in ea10e03.
Testing instructions:
Verify that embeds preview or don't preview, depending on whether the embed type can support previews:
Previewable example:
Non-previewable examples: