-
Notifications
You must be signed in to change notification settings - Fork 32
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
Fix rendering errors when rendering multiple recommendation widgets #397
Conversation
@@ -15,7 +15,6 @@ | |||
.parsely-recommended-widget-entry img { | |||
float: left; | |||
margin-right: 15px; | |||
max-width: 50%; |
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.
How does this change fit in?
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.
Styles weren't applied correctly in the past and this would result in the image looking too small with the changes in this PR.
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 can see how different widget configs would not conflict with each other. ✅
What happens when the JS it blocked / not available / fails? Does the div that is now hard-coded in the HTML break any layouts with the default styles?
…into fix/multiple-widgets
@GaryJones If Javascript is disabled, the contents don't load. The only thing that gets rendered is an empty div that doesn't affect the rest of the page. |
|
||
<div class="parsely-recommended-widget" | ||
data-parsely-widget-display-author="<?php echo esc_attr( isset( $instance['display_author'] ) ? wp_json_encode( boolval( $instance['display_author'] ) ) : false ); ?>" | ||
data-parsely-widget-display-direction="<?php echo esc_attr( isset( $instance['display_direction'] ) ? $instance['display_direction'] : null ); ?>" |
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.
Instead of adding null
string to the attribute, could an empty string be used instead? Just seems cleaner.
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.
✅
Description
If multiple recommendation widgets were set up in a page, only the last one would render. That was caused because the script responsible of rendering the widget was enqueued multiple times, but it only the last enqueue would survive.
This PR changes the logic slightly: the script is still enqueued multiple times, but we take advantage of the fact that it's actually called only once. To render the appropriate widgets, we render an empty div from the PHP side and pass the data to javascript using
data
attributes. Then, the script reads all thediv
with that attributes and renders the actual widget.Motivation and Context
Not exactly this issue, but related: #206
How Has This Been Tested?
Rendered multiple widgets with the same and different configurations in the page.
Screenshots (if appropriate):
Before
After
Types of changes
Bug fix (non-breaking change which fixes an issue)