Skip to content

Eventbrite, Pinterest Blocks: Use new shared function to test embeddable URLs#15218

Merged
Copons merged 4 commits intomasterfrom
update/eventbrite-pinterest-resolve-redirect
Apr 6, 2020
Merged

Eventbrite, Pinterest Blocks: Use new shared function to test embeddable URLs#15218
Copons merged 4 commits intomasterfrom
update/eventbrite-pinterest-resolve-redirect

Conversation

@Copons
Copy link
Copy Markdown
Contributor

@Copons Copons commented Mar 31, 2020 •
edited
Loading

Fixes #14439
Fixes #14896

Changes proposed in this Pull Request:

  • Use the testEmbedUrl shared function on Eventbrite and Pinterest.
  • Use withNotices to warn when the provided URL is not embeddable.

⚠️ NOTE: I've commited with --no-verify because of a current issue with ESLint, that throws an error when parsing template literals.

Testing instructions:

Eventbrite:

Pinterest:

  • Perform the same tests with the provided URLs for both the Eventbrite and Pinterest blocks.
  • Add a block and try embedding the correct URL: make sure it's embedded correctly.
  • Add a block and try embedding the wrong URL: make sure it reverts to placeholder, with a notice offering to convert the URL to a link. Also make sure the conversion to link works as expected.
  • Try pasting the correct URL in the editor: make sure it's converted into its block, and successfully embedded.
  • Try pasting the wrong URL: make sure it's converted into its block, but to placeholder state with the error notice.
  • Only for Eventbrite:
    • Add two blocks, both with the correct URL. Turn one of the blocks into a normal button (embed style "modal").
    • Save and reload.
    • Make sure the "embedding" spinner only shows up for the actual embedded version of the block, while the button/modal version appears immediately without loading.

Proposed changelog entry for your changes:

  • N/A (mostly behind the scene changes)

@Copons Copons added [Type] Janitorial [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Pinterest [Block] Eventbrite [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Mar 31, 2020
@Copons Copons requested review from a team March 31, 2020 17:22
@Copons Copons self-assigned this Mar 31, 2020
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 31, 2020

Caution: This PR has changes that must be merged to WordPress.com
Hello Copons! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D41155-code before merging this PR. Thank you!

@Copons Copons mentioned this pull request Mar 31, 2020
Merged
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Mar 31, 2020 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against cd045e1

@jeherve jeherve force-pushed the update/eventbrite-pinterest-resolve-redirect branch from a98be0a to 6d5bf6d Compare April 1, 2020 15:20
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Apr 1, 2020

Copons, Your synced wpcom patch D41155-code has been updated.

jeherve
jeherve previously approved these changes Apr 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me. I rebased to address some unrelated issues with e2e tests that were causing Travis tests to fail.

@jeherve jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 1, 2020
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Apr 1, 2020

It looks like the Pinterest and EventBrite tests are still failing:
https://travis-ci.org/github/Automattic/jetpack/jobs/669750106?utm_medium=notification&utm_source=github_status

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Team Review Obsolete. Use Needs Review instead. [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 1, 2020
@Copons Copons force-pushed the update/eventbrite-pinterest-resolve-redirect branch from 6d5bf6d to 1b3e2a4 Compare April 2, 2020 11:38
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Apr 2, 2020

Copons, Your synced wpcom patch D41155-code has been updated.

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Apr 3, 2020

Copons, Your synced wpcom patch D41155-code has been updated.

@glendaviesnz
Copy link
Copy Markdown
Contributor

glendaviesnz commented Apr 3, 2020

@Copons & @jeherve - I fixed the e2e tests - it mostly needed a wait adding to allow the url to resolve - but also resolving the url seemed to changed the embed on the frontend for pinterest, so selector for that needed changing.

I also modified the pinterest one to allow the pinId to be passed into the page object to match the way the newer eventbrite one was working while I was at it.

glendaviesnz
glendaviesnz reviewed Apr 3, 2020
View reviewed changes
extensions/blocks/eventbrite/edit.js
};

this.setState( { resolvedStatusCode: null } );
testEmbedUrl( newAttributes.url, this.setIsResolvingUrl )
Copy link
Copy Markdown
Contributor

@glendaviesnz glendaviesnz Apr 3, 2020

Choose a reason for hiding this comment

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

for a follow up PR at this stage, but I wonder if we convert this to a functional component if we can abstract this further with a custom hook?

Copy link
Copy Markdown
Contributor Author

@Copons Copons Apr 3, 2020

Choose a reason for hiding this comment

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

Oh, that's interesting, I've never created a custom hook, so I didn't think of doing something like that!

@Copons Copons added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Apr 3, 2020
@Copons Copons requested a review from jeherve April 3, 2020 11:00
@Copons
Copy link
Copy Markdown
Contributor Author

Copons commented Apr 3, 2020

@jeherve Thanks to @glendaviesnz's change Travis is now passing! 🎉

@jeherve jeherve added this to the 8.5 milestone Apr 6, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Apr 6, 2020
@Copons Copons merged commit ac86337 into master Apr 6, 2020
@Copons Copons deleted the update/eventbrite-pinterest-resolve-redirect branch April 6, 2020 09:49
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 6, 2020
@Copons
Copy link
Copy Markdown
Contributor Author

Copons commented Apr 6, 2020

r205396-wpcom

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

Reviewers

@glendaviesnz glendaviesnz glendaviesnz left review comments

@jeherve jeherve jeherve approved these changes

Assignees

@Copons Copons

Labels

[Block] Eventbrite [Block] Pinterest [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Janitorial

Projects

None yet

Milestone

8.5

Development

Successfully merging this pull request may close these issues.

Eventbrite and Pinterest: Use the withNotices HOC Eventbrite/Pinterest Blocks: Extract resolveRedirect as shared utility function

5 participants

@Copons @matticbot @jetpackbot @jeherve @glendaviesnz