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

Ensure consistent header and sub header spacing between block and shortcode output #41

Merged
merged 4 commits into from
Jun 28, 2024

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Jun 28, 2024

Description of the Change

As described here, there's a slight difference in spacing when using the shortcode block vs the normal block. This will vary based on the theme you're using, as this spacing isn't something we are adding. But in testing some of the core themes, what's happening here is the shortcode output does not have a wrapping container around things and there's some styling that gets applied to all direct children of the main content area.

This PR fixes things by adding in a wrapping container if a custom container isn't being used (when used as a normal widget in a widget area, there is already a wrapper so we don't want to double wrap). This removes this extra spacing. We also are adding some top margin on the sub-header to match the bottom margin that already existed there.

Partially closes #37

Block output Shortcode output

How to test the Change

  1. Install, activate and configure the plugin
  2. Create a new page and add a Mailchimp Block and a Shortcode Block with the Mailchimp shortcode
  3. View things on the front-end and note that there should be a container around both blocks and the spacing between the header and sub-header should match
  4. If desired, use a widget enabled theme and add the Mailchimp widget into a widget area. Ensure things display correctly there as well

Changelog Entry

Fixed - Ensure the custom block and shortcode both have consistent spacing

Credits

Props @dkotter, @qasumitbagthariya

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

dkotter added 2 commits June 28, 2024 10:19
…g inconsistencies between the shortcode and block output. Add a top margin to match the bottom margin of the sub header
…lds don't exist. This prevents an extra layer around a normal widget
@dkotter dkotter added this to the 1.6.0 milestone Jun 28, 2024
@dkotter dkotter self-assigned this Jun 28, 2024
@dkotter dkotter requested a review from nateconley June 28, 2024 16:28
@github-actions github-actions bot added the needs:code-review This requires code review. label Jun 28, 2024
Copy link
Collaborator

@nateconley nateconley left a comment

Choose a reason for hiding this comment

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

@dkotter This works really well. Just one question around a possible edge case.

echo wp_kses_post( $after_widget );
if ( ! empty( $after_widget ) ) {
echo wp_kses_post( $after_widget );
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we should check for before_widget for the closing</div>, in the rare case that these args are being used for self-closing tags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a good point. There may be a scenario (hopefully edge case) where $before_widget is empty but $after_widget isn't (or vice versa) and we could end up having an opening tag with no closing or closing tag with no opening. Just pushed a fix that I think accounts for this: e9a2dc7

@dkotter dkotter requested a review from nateconley June 28, 2024 21:05
Copy link
Collaborator

@nateconley nateconley left a comment

Choose a reason for hiding this comment

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

Your latest change handles that edge case well.

@dkotter dkotter merged commit d6bc770 into develop Jun 28, 2024
3 checks passed
@dkotter dkotter deleted the fix/header-spacing branch June 28, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression Testing for 1.6.0
2 participants