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

Calendly: Removed look behinds in regexes #14375

Merged
merged 2 commits into from
Jan 17, 2020
Merged

Calendly: Removed look behinds in regexes #14375

merged 2 commits into from
Jan 17, 2020

Conversation

pablinos
Copy link
Contributor

Changes proposed in this Pull Request:

Look behinds aren't supported by Firefox, IE and some other browsers, which prevented the Calendly block from registering correctly.

This change replaces those regular expressions with ones that use groups to grab what is needed from the string.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This fixes a bug with the soon to be launched Calendly block

Testing instructions:

With a Jetpack site, activate the beta blocks and test that the different types of embed code can still be used with the Calendly block.

Optionally test it in another browser that doesn't support look behinds e.g. Firefox.

Proposed changelog entry for your changes:

  • N/A

This isn't supported by Firefox, IE and possibly other browsers
@pablinos pablinos added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Block] Calendly labels Jan 16, 2020
@pablinos pablinos requested a review from a team January 16, 2020 21:34
@pablinos pablinos self-assigned this Jan 16, 2020
@matticbot
Copy link
Contributor

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

@jetpackbot
Copy link

jetpackbot commented Jan 16, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 23, 2020.
Scheduled code freeze: January 16, 2020

Generated by 🚫 dangerJS against 8d1c636

@pablinos
Copy link
Contributor Author

Fixes #14376

@jeherve jeherve added this to the 8.2 milestone Jan 17, 2020
@jeherve jeherve added the [Type] Bug When a feature is broken and / or not performing as intended label Jan 17, 2020
Copy link
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.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 17, 2020
The new regex wasn't taking into account the case where the embed code
could be a bare URL starting with `calendly.com`
@matticbot
Copy link
Contributor

pablinos, Your synced wpcom patch D37816-code has been updated.

@pablinos
Copy link
Contributor Author

Whoops! Thanks @jeherve. That should be fixed now.

Copy link
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.

Let's give this a try. It seems to work on my end!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 17, 2020
@pablinos pablinos merged commit 03212a4 into master Jan 17, 2020
@pablinos pablinos deleted the fix/calendly-regex branch January 17, 2020 18:08
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Calendly Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants