Skip to content

Calendly: Changed the embed, to remove the inline styles #14579

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

Jump to bottom
Merged
pablinos merged 9 commits into from
Feb 26, 2020
Merged

Calendly: Changed the embed, to remove the inline styles #14579

pablinos merged 9 commits into from
Feb 26, 2020

Conversation

@pablinos
Copy link
Contributor

@pablinos pablinos commented Feb 5, 2020

The inline styles added to the parent container by the Calendly script
were interfering with the CSS used for aligning elements to the left
and right on the desktop view of Twenty Twenty (and so probably other
themes)

Changes proposed in this Pull Request:

An undocumented option to the init function stops these styles being
added, and so this change swaps the embed code to set that option, as
well as tweak the CSS to make the best effort at laying out the embed when
aligned to the sides.

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

This an improvement on a new feature

Testing instructions:

  • Embed a Calendly block and set the alignment to left or right
  • Add some lorem ipsum text to the post
  • Preview the post at different screen sizes. (You'll have to reload the page sometimes as the Calendly code sets the width of the embed on load and doesn't always resize it)
  • Check that the embed isn't (too) broken. Previously the embed would disappear over screens 1000px wide and be partially cut off by the side of the screen.

There are still some known problems with the Twenty Twenty theme and alignment of blocks, but having discussed it with @kjellr (p1580823343135400-slack-themes), we've decided that it's best to leave the layout to the theme and make the block as lightweight as possible.

Proposed changelog entry for your changes:

  • No changelog required

@pablinos pablinos added [Status] Needs Review This PR is ready for review. [Block] Calendly labels Feb 5, 2020
@pablinos pablinos requested review from a team and scruffian February 5, 2020 16:01
@pablinos pablinos self-assigned this Feb 5, 2020
@jetpackbot
Copy link
Collaborator

jetpackbot commented Feb 5, 2020 •
edited
Loading

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: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against 22b52ac

@scruffian
Copy link
Member

scruffian commented Feb 10, 2020

This looks good to me, just two thoughts:

  1. If this feature is undocumented is it a good idea to depend on it?

  2. As you said, when you resize the window the layout gets stuck:

Screenshot 2020-02-10 at 12 58 12

I wonder if we can fire an event on resize to sort this out? I added an issue for it: #14619

scruffian
scruffian previously approved these changes Feb 10, 2020
View reviewed changes
@pablinos
Copy link
Contributor Author

pablinos commented Feb 10, 2020

  • If this feature is undocumented is it a good idea to depend on it?

Well, no, but there's no other option really. If we allow it to add the inline styles it breaks things, and to override it, we'd probably have to get into a lot of theme-specific CSS, or sprinkling !important around, which doesn't seem like a good idea either.

  • As you said, when you resize the window the layout gets stuck:

Yes, I don't think there's much we can do about that at the moment. It's all wrapped up with the problem of alignment and the way these aligned embeds are positioned using absolute.

@jeherve jeherve added this to the 8.3 milestone Feb 10, 2020
@jeherve jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Feb 10, 2020
@pablinos pablinos added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Feb 11, 2020
@scruffian scruffian force-pushed the update/calendly-embed-code branch from 834381b to 366bc2c Compare February 11, 2020 19:39
jeherve
jeherve requested changes Feb 24, 2020
View reviewed changes
@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] In Progress labels Feb 24, 2020
jeherve
jeherve requested changes Feb 24, 2020
View reviewed changes
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.

This works for me, I only have minor comments left.

@jeherve jeherve removed this from the 8.3 milestone Feb 24, 2020
pablinos and others added 8 commits February 25, 2020 12:59
@pablinos @scruffian
Changed the embed, to remove the inline styles
a6f76fd 
The inline styles added to the parent container by the Calendly script
were interferring with the CSS used for aligning elements to the left
and right on the desktop view of Twenty Twenty (and so probably other
themes)

An undocumented option to the init function stops these styles being
added, and so this change swaps the embed code to set that option, as
well as tweak the CSS to make a best effort at laying out the embed when
aligned to the sides.
@pablinos @scruffian
Scoped the height setting to the inline style
b61525b
@pablinos @scruffian
Prevent the server side rendering in the admin
5014c68 
The render_callback was being called to get the content of the page in
the admin, and this was causing the block embed to render full screen
when it was inline mode. This short circuits the render callback in the
admin.
@pablinos @scruffian
Simplify the block ID code, and remove is_admin check
fcf9f40 
We've discovered using `wp_unique_id`, so it seemed sensible to use that
here.

There was a check for is_admin in the render callback, because of an
error with the post being rendered as the editor loaded. I can't
replicate this now, so I've removed the check.
@pablinos @scruffian
Changed the embed, to remove the inline styles
9d80b35 
The inline styles added to the parent container by the Calendly script
were interferring with the CSS used for aligning elements to the left
and right on the desktop view of Twenty Twenty (and so probably other
themes)

An undocumented option to the init function stops these styles being
added, and so this change swaps the embed code to set that option, as
well as tweak the CSS to make a best effort at laying out the embed when
aligned to the sides.
@pablinos @scruffian
Scoped the height setting to the inline style
bdf11e2
@pablinos @scruffian
Prevent the server side rendering in the admin
5b5eafc 
The render_callback was being called to get the content of the page in
the admin, and this was causing the block embed to render full screen
when it was inline mode. This short circuits the render callback in the
admin.
@scruffian
fix rebase
b829347
@scruffian scruffian force-pushed the update/calendly-embed-code branch from 001c029 to b829347 Compare February 25, 2020 13:00
@scruffian scruffian 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 Feb 25, 2020
@scruffian
Copy link
Member

scruffian commented Feb 25, 2020

Ready for another review.

@jeherve jeherve added this to the 8.4 milestone Feb 25, 2020
jeherve
jeherve approved these changes Feb 25, 2020
View reviewed changes
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.

This works well in my tests. 👍

@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 Feb 25, 2020
@matticbot
Copy link
Contributor

matticbot commented Feb 26, 2020

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 D39422-code before merging this PR. Thank you!

@pablinos pablinos merged commit 51bbad7 into master Feb 26, 2020
@pablinos pablinos deleted the update/calendly-embed-code branch February 26, 2020 13:03
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 26, 2020
@pablinos
Copy link
Contributor Author

pablinos commented Feb 26, 2020

r203460-wpcom

@brbrr brbrr mentioned this pull request Mar 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scruffian scruffian scruffian left review comments

@jeherve jeherve jeherve approved these changes

Assignees

@pablinos pablinos

Labels

[Block] Calendly Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Milestone

8.4

Development

Successfully merging this pull request may close these issues.

6 participants

@pablinos @jetpackbot @scruffian @matticbot @jeherve