-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
…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
There was a problem hiding this 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.
mailchimp_widget.php
Outdated
echo wp_kses_post( $after_widget ); | ||
if ( ! empty( $after_widget ) ) { | ||
echo wp_kses_post( $after_widget ); | ||
} else { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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.
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
How to test the Change
Changelog Entry
Credits
Props @dkotter, @qasumitbagthariya
Checklist: