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.

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

Eventbrite Block: Add alignment support #14817

Merged
merged 6 commits into from
Mar 5, 2020
Merged

Eventbrite Block: Add alignment support #14817

merged 6 commits into from
Mar 5, 2020

Conversation

apeatling
Copy link
Member

Changes proposed in this Pull Request:

Add alignment support to the Eventbrite block. This works for both the modal button and in-page embed styles.

Fixes #14505

Screen Shot 2020-02-26 at 10 20 40 AM

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

  • This fixes an existing feature.

Testing instructions:

  • Add an Eventbrite block to a post or page, and add an event (example: https://www.eventbrite.ca/e/5th-construction-expo-2020-tickets-31941745621)
  • Confirm you can see the "left, center, right" alignment option in the block settings bar when using the in-page embed style.
  • Confirm you can also see alignment options when the Button & Modal style is selected
  • Choose left, right, and center alignment options, and confirm these work correctly in the editor.

Proposed changelog entry for your changes:

  • None.

@apeatling apeatling added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review This PR is ready for review. labels Feb 26, 2020
@apeatling apeatling requested review from creativecoder and a team February 26, 2020 18:21
@apeatling apeatling self-assigned this Feb 26, 2020
@matticbot
Copy link
Contributor

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

@jetpackbot
Copy link
Collaborator

jetpackbot commented Feb 26, 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: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against f7dfb6e

@creativecoder
Copy link
Contributor

@apeatling I'd recommend adding the Explorers Github team as a reviewer for broader exposure

@creativecoder
Copy link
Contributor

Just noting that the align options in the Calendly block have some issues with a wrapper div being removed from Gutenberg since the most recent WordPress release. I'm not sure if that effects Eventbrite--please be sure to test this one with and without the Gutenberg plugin activated.

@jeherve jeherve added this to the 8.4 milestone Feb 27, 2020
@apeatling apeatling requested a review from a team February 27, 2020 16:04
@apeatling
Eventbrite Alignment: Add position relative to block to make sure tha…
ca78d71 
…t the iframe cannot be interacted with when alignment is set. See #14820
@matticbot
Copy link
Contributor

apeatling, Your synced wpcom patch D39459-code has been updated.

@apeatling
Copy link
Member Author

@creativecoder Thanks for highlighting that. It was also an issue here and I've added the same fix to this block. Working correctly now.

pablinos
pablinos previously approved these changes Feb 27, 2020
View reviewed changes
Copy link
Contributor

@pablinos pablinos 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 for me!

@scruffian
Copy link
Member

This doesn't work for me when in the "in-page embed" mode. I'm happy to fix this if you don't mind me making changes to your PR? Otherwise I'm happy to let you do it :)

scruffian
scruffian previously requested changes Mar 2, 2020
View reviewed changes
Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

The In-page embed mode isn't working in the front end.

@scruffian scruffian 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 Review This PR is ready for review. labels Mar 2, 2020
@apeatling
Copy link
Member Author

@scruffian Feel free to jump in on these PRs, no issue!

@matticbot
Copy link
Contributor

apeatling, Your synced wpcom patch D39459-code has been updated.

@apeatling
Copy link
Member Author

@scruffian Pushed a fix for the front end alignment. Feel free to adjust if there's a better way.

@apeatling apeatling requested a review from scruffian March 2, 2020 22:15
@apeatling apeatling 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 Mar 2, 2020
@matticbot
Copy link
Contributor

apeatling, Your synced wpcom patch D39459-code has been updated.

@scruffian
Copy link
Member

I made a few changes:

  1. We should be supporting the additional classes users can add in the sidebar, which means its best to use the Jetpack_Gutenberg::block_classes helper function. This also simplifies things a lot
  2. This means we should also wrap the classes in esc_attr in case someone adds something silly.
  3. I also updated the class name to reflect what that helper function returns.

@apeatling
Copy link
Member Author

@scruffian Thanks! TIL about Jetpack_Gutenberg::block_classes.

@apeatling apeatling requested a review from a team March 3, 2020 18:44
@scruffian scruffian dismissed their stale review March 4, 2020 10:37

PR fixed

scruffian
scruffian previously approved these changes Mar 4, 2020
View reviewed changes
Copy link
Member

@scruffian scruffian 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 now, but someone else should probably review it since I wrote a lot of this code :)

@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 Review This PR is ready for review. labels Mar 4, 2020
@matticbot
Copy link
Contributor

apeatling, Your synced wpcom patch D39459-code has been updated.

@scruffian
Copy link
Member

I added support for wide and full alignments. Here's how they look:
Screenshot 2020-03-04 at 16 09 15
Screenshot 2020-03-04 at 16 08 07
It's not ideal that the iframe content is restricted in width, but it feels like it's ok to offer these options if someone wants them...

Copons
Copons approved these changes Mar 4, 2020
View reviewed changes
Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

This seems to work fine for me.
I gave a look at Eventbrite's docs to see if there's a way to control the embedded max width, but apparently not, and so we are stuck to 1080px max.

While I'm not super into the gray background, we also need to consider that the effect would wildly vary based both on the screen size and the theme sizes.
What looks a bit off on Twenty Twenty on desktop, might look better on another theme, or another device.

@apeatling
Copy link
Member Author

Looks good thanks for the update @scruffian. 👍

@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 Mar 5, 2020
jeherve
jeherve approved these changes Mar 5, 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.

LGTM! 👍

@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 Mar 5, 2020
@scruffian scruffian merged commit bf3e4cc into master Mar 5, 2020
@scruffian scruffian deleted the add/eventbrite-button-alignment branch March 5, 2020 17:10
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 5, 2020
@scruffian
Copy link
Member

r203918-wpcom

jeherve added a commit that referenced this pull request Mar 20, 2020
@jeherve
Changelog: add #14817
cb39e54
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
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

@Copons Copons Copons approved these changes

@pablinos pablinos pablinos left review comments

@creativecoder creativecoder Awaiting requested review from creativecoder

Assignees

@apeatling apeatling

Labels
[Block] Eventbrite 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.

Eventbrite Block: Add alignment options for modal button
8 participants
@apeatling @matticbot @jetpackbot @creativecoder @scruffian @pablinos @jeherve @Copons